Fix: Images cannot show when projects' path was changed

CHANGELOG

@@ -33,6 +33,7 @@ v 8.1.0 (unreleased)
   - Add user preference to change layout width (Peter Göbel)
   - Use commit status in merge request widget as preffered source of CI status
   - Integrate CI commit and build pages into project pages
+  - Fix: Images cannot show when projects' path was changed
 v 8.0.4
   - Fix Message-ID header to be RFC 2111-compliant to prevent e-mails being dropped (Stan Hu)

app/models/project.rb

@@ -636,6 +636,13 @@ class Project < ActiveRecord::Base
       # db changes in order to prevent out of sync between db and fs
       raise Exception.new('repository cannot be renamed')
     end
+
+    begin
+      move_uploads(namespace, path_was, path)
+    rescue Exception => e
+      Rails.logger.error(e.message)
+      false
+    end
   end
   def hook_attrs
@@ -760,4 +767,11 @@ class Project < ActiveRecord::Base
     service.active = true
     service.save
   end
+
+  def move_uploads(namespace, old_path, new_path)
+    base_dir = File.join(Rails.root, "public", "uploads", namespace.path)
+    from = File.join(base_dir, old_path)
+    to = File.join(base_dir, new_path)
+    FileUtils.mv(from, to)
+  end
 end

app/services/projects/transfer_service.rb

@@ -27,6 +27,7 @@ module Projects
     def transfer(project, new_namespace)
       Project.transaction do
         old_path = project.path_with_namespace
+        old_namespace = project.namespace
         new_path = File.join(new_namespace.try(:path) || '', project.path)
         if Project.where(path: project.path, namespace_id: new_namespace.try(:id)).present?
@@ -51,6 +52,9 @@ module Projects
         # clear project cached events
         project.reset_events_cache
+        # Move uploads
+        move_uploads_to_new_namespace(old_namespace.path, new_namespace.path)
+
         true
       end
     end
@@ -61,5 +65,14 @@ module Projects
         namespace.id != project.namespace_id &&
         current_user.can?(:create_projects, namespace)
     end
+
+    private
+
+    def move_uploads_to_new_namespace(old_path, new_path)
+      base_dir = File.join(Rails.root, "public", "uploads")
+      from = File.join(base_dir, old_path)
+      to = File.join(base_dir, new_path)
+      FileUtils.mv(from, to)
+    end
   end
 end

app/uploaders/file_uploader.rb

@@ -26,7 +26,7 @@ class FileUploader < CarrierWave::Uploader::Base
   end
   def secure_url
-    File.join(Gitlab.config.gitlab.url, @project.path_with_namespace, "uploads", @secret, file.filename)
+    File.join("/uploads", @secret, file.filename)
   end
   def file_storage?

features/steps/admin/projects.rb

@@ -41,6 +41,8 @@ class Spinach::Features::AdminProjects < Spinach::FeatureSteps
   end
   step 'I transfer project to group \'Web\'' do
+    allow_any_instance_of(Projects::TransferService).
+      to receive(:move_uploads_to_new_namespace).and_return(true)
     find(:xpath, "//input[@id='new_namespace_id']").set group.id
     click_button 'Transfer'
   end

lib/gitlab/markdown.rb

@@ -41,6 +41,7 @@ module Gitlab
     autoload :IssueReferenceFilter,         'gitlab/markdown/issue_reference_filter'
     autoload :LabelReferenceFilter,         'gitlab/markdown/label_reference_filter'
     autoload :MergeRequestReferenceFilter,  'gitlab/markdown/merge_request_reference_filter'
+    autoload :UploadLinkFilter,             'gitlab/markdown/upload_link_filter'
     autoload :RelativeLinkFilter,           'gitlab/markdown/relative_link_filter'
     autoload :SanitizationFilter,           'gitlab/markdown/sanitization_filter'
     autoload :SnippetReferenceFilter,       'gitlab/markdown/snippet_reference_filter'
@@ -140,6 +141,7 @@ module Gitlab
         Gitlab::Markdown::SyntaxHighlightFilter,
         Gitlab::Markdown::SanitizationFilter,
+        Gitlab::Markdown::UploadLinkFilter,
         Gitlab::Markdown::RelativeLinkFilter,
         Gitlab::Markdown::EmojiFilter,
         Gitlab::Markdown::TableOfContentsFilter,

lib/gitlab/markdown/upload_link_filter.rb

+require 'gitlab/markdown'
+require 'html/pipeline/filter'
+require 'uri'
+
+module Gitlab
+  module Markdown
+    # HTML filter that "fixes" relative upload links to files.
+
+    class UploadLinkFilter < HTML::Pipeline::Filter
+      def call
+        doc.search('a').each do |el|
+          process_link_attr el.attribute('href')
+        end
+
+        doc.search('img').each do |el|
+          process_link_attr el.attribute('src')
+        end
+
+        doc
+      end
+
+      protected
+
+      def process_link_attr(html_attr)
+        return if html_attr.blank?
+
+        uri = html_attr.value
+        if uri.starts_with?("/uploads/")
+          html_attr.value = build_url(uri).to_s
+        end
+      end
+
+      def build_url(uri)
+        File.join(Gitlab.config.gitlab.url, context[:project].path_with_namespace, uri)
+      end
+    end
+  end
+end

spec/controllers/projects/uploads_controller_spec.rb

@@ -33,7 +33,7 @@ describe Projects::UploadsController do
       it 'returns a content with original filename, new link, and correct type.' do
         expect(response.body).to match '\"alt\":\"rails_sample\"'
-        expect(response.body).to match "\"url\":\"http://localhost/#{project.path_with_namespace}/uploads"
+        expect(response.body).to match "\"url\":\"/uploads"
         expect(response.body).to match '\"is_image\":true'
       end
     end
@@ -49,7 +49,7 @@ describe Projects::UploadsController do
       it 'returns a content with original filename, new link, and correct type.' do
         expect(response.body).to match '\"alt\":\"doc_sample.txt\"'
-        expect(response.body).to match "\"url\":\"http://localhost/#{project.path_with_namespace}/uploads"
+        expect(response.body).to match "\"url\":\"/uploads"
         expect(response.body).to match '\"is_image\":false'
       end
     end

spec/lib/gitlab/email/attachment_uploader_spec.rb

@@ -13,7 +13,6 @@ describe Gitlab::Email::AttachmentUploader do
       expect(link).not_to be_nil
       expect(link[:is_image]).to be_truthy
       expect(link[:alt]).to eq("bricks")
-      expect(link[:url]).to include("/#{project.path_with_namespace}")
       expect(link[:url]).to include("bricks.png")
     end
   end

spec/lib/gitlab/markdown/upload_link_filter_spec.rb

+# encoding: UTF-8
+
+require 'spec_helper'
+
+module Gitlab::Markdown
+  describe UploadLinkFilter do
+    def filter(doc, contexts = {})
+      contexts.reverse_merge!({
+        project: project
+      })
+
+      described_class.call(doc, contexts)
+    end
+
+    def image(path)
+      %(<img src="#{path}" />)
+    end
+
+    def link(path)
+      %(<a href="#{path}">#{path}</a>)
+    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.path_with_namespace}/uploads/e90decf88d8f96fe9e1389afc2e4a91f/test.jpg"
+      end
+
+      it 'rebuilds relative URL for an image' do
+        doc = filter(link('/uploads/e90decf88d8f96fe9e1389afc2e4a91f/test.jpg'))
+        expect(doc.at_css('a')['href']).
+          to eq "#{Gitlab.config.gitlab.url}/#{project.path_with_namespace}/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.path_with_namespace}/uploads/%ED%95%9C%EA%B8%80.png"
+      end
+    end
+  end
+end

spec/services/projects/download_service_spec.rb

@@ -37,7 +37,6 @@ describe Projects::DownloadService do
         it { expect(@link_to_file).to have_key('url') }
         it { expect(@link_to_file).to have_key('is_image') }
         it { expect(@link_to_file['is_image']).to be true }
-        it { expect(@link_to_file['url']).to match("/#{@project.path_with_namespace}") }
         it { expect(@link_to_file['url']).to match('rails_sample.jpg') }
         it { expect(@link_to_file['alt']).to eq('rails_sample') }
       end
@@ -52,7 +51,6 @@ describe Projects::DownloadService do
         it { expect(@link_to_file).to have_key('url') }
         it { expect(@link_to_file).to have_key('is_image') }
         it { expect(@link_to_file['is_image']).to be false }
-        it { expect(@link_to_file['url']).to match("/#{@project.path_with_namespace}") }
         it { expect(@link_to_file['url']).to match('doc_sample.txt') }
         it { expect(@link_to_file['alt']).to eq('doc_sample.txt') }
       end

spec/services/projects/transfer_service_spec.rb

@@ -7,6 +7,8 @@ describe Projects::TransferService do
   context 'namespace -> namespace' do
     before do
+      allow_any_instance_of(Projects::TransferService).
+        to receive(:move_uploads_to_new_namespace).and_return(true)
       group.add_owner(user)
       @result = transfer_project(project, user, group)
     end

spec/services/projects/upload_service_spec.rb

@@ -18,7 +18,6 @@ describe Projects::UploadService do
       it { expect(@link_to_file).to have_key(:is_image) }
       it { expect(@link_to_file).to have_value('banana_sample') }
       it { expect(@link_to_file[:is_image]).to equal(true) }
-      it { expect(@link_to_file[:url]).to match("/#{@project.path_with_namespace}") }
       it { expect(@link_to_file[:url]).to match('banana_sample.gif') }
     end
@@ -34,7 +33,6 @@ describe Projects::UploadService do
       it { expect(@link_to_file).to have_value('dk') }
       it { expect(@link_to_file).to have_key(:is_image) }
       it { expect(@link_to_file[:is_image]).to equal(true) }
-      it { expect(@link_to_file[:url]).to match("/#{@project.path_with_namespace}") }
       it { expect(@link_to_file[:url]).to match('dk.png') }
     end
@@ -49,7 +47,6 @@ describe Projects::UploadService do
       it { expect(@link_to_file).to have_key(:is_image) }
       it { expect(@link_to_file).to have_value('rails_sample') }
       it { expect(@link_to_file[:is_image]).to equal(true) }
-      it { expect(@link_to_file[:url]).to match("/#{@project.path_with_namespace}") }
       it { expect(@link_to_file[:url]).to match('rails_sample.jpg') }
     end
@@ -64,7 +61,6 @@ describe Projects::UploadService do
       it { expect(@link_to_file).to have_key(:is_image) }
       it { expect(@link_to_file).to have_value('doc_sample.txt') }
       it { expect(@link_to_file[:is_image]).to equal(false) }
-      it { expect(@link_to_file[:url]).to match("/#{@project.path_with_namespace}") }
       it { expect(@link_to_file[:url]).to match('doc_sample.txt') }
     end