Merge branch '52650-webhook-image-urls-rewritten-incorrectly-in-issues' into 'master'

Resolve "Webhook Image URLs Rewritten Incorrectly in Issues"

Closes #52650

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

changelogs/unreleased/test-usage-ping-in-timeout-case.yml

@@ -0,0 +1,5 @@
+---
+title: Fix auto-corrected upload URLs in webhooks
+merge_request: 22361
+author:
+type: fixed

doc/user/project/integrations/webhooks.md

@@ -7,7 +7,7 @@
 > - the `project.http_url` key is deprecated in favor of the `project.git_http_url` key
 >
 > **Note:**
-> Starting from GitLab 11.1, the logs of web hooks are automatically removed after
+> Starting from GitLab 11.1, the logs of webhooks are automatically removed after
 > one month.
 >
 > **Note:**
@@ -73,7 +73,7 @@ Below are described the supported events.
 
 Triggered when you push to the repository except when pushing tags.
 
-> **Note:** When more than 20 commits are pushed at once, the `commits` web hook 
+> **Note:** When more than 20 commits are pushed at once, the `commits` webhook
   attribute will only contain the first 20 for performance reasons. Loading
   detailed commit data is expensive. Note that despite only 20 commits being
   present in the `commits` attribute, the `total_commits_count` attribute will
@@ -1157,10 +1157,11 @@ its description:
 ```
 
 It will appear in the webhook body as the below (assuming that GitLab is
-installed at gitlab.example.com):
+installed at gitlab.example.com, and the project is at
+example-group/example-project):
 
 ```markdown
-![image](https://gitlab.example.com/uploads/$sha/image.png)
+![image](https://gitlab.example.com/example-group/example-project/uploads/$sha/image.png)
 ```
 
 This will not rewrite URLs that already are pointing to HTTP, HTTPS, or
@@ -1190,7 +1191,7 @@ From this page, you can repeat delivery with the same data by clicking `Resend R
 
 > **Note:** If URL or secret token of the webhook were updated, data will be delivered to the new address.
 
-### Receiving duplicate or multiple web hook requests triggered by one event
+### Receiving duplicate or multiple webhook requests triggered by one event
 
 When GitLab sends a webhook it expects a response in 10 seconds (set default value). If it does not receive one, it'll retry the webhook.
 If the endpoint doesn't send its HTTP response within those 10 seconds, GitLab may decide the hook failed and retry it.

lib/gitlab/hook_data/base_builder.rb

@@ -25,6 +25,7 @@ module Gitlab
         markdown_text.gsub(MARKDOWN_SIMPLE_IMAGE) do
           if $~[:image]
             url = $~[:url]
+            url = "#{uploads_prefix}#{url}" if url.start_with?('/uploads')
             url = "/#{url}" unless url.start_with?('/')
 
             "![#{$~[:title]}](#{Gitlab.config.gitlab.url}#{url})"
@@ -33,6 +34,16 @@ module Gitlab
           end
         end
       end
+
+      def uploads_prefix
+        project&.full_path || ''
+      end
+
+      def project
+        return unless object.respond_to?(:project)
+
+        object.project
+      end
     end
   end
 end

spec/lib/gitlab/hook_data/base_builder_spec.rb

@@ -8,16 +8,52 @@ describe Gitlab::HookData::BaseBuilder do
       end
     end
 
-    subject { subclass.new(nil) }
-
     using RSpec::Parameterized::TableSyntax
 
+    context 'with an upload prefix specified' do
+      let(:project_with_path) { double(full_path: 'baz/bar') }
+      let(:object_with_project) { double(project: project_with_path) }
+      subject { subclass.new(object_with_project) }
+
       where do
         {
           'relative image URL' => {
             input: '![an image](foo.png)',
             output: "![an image](#{Gitlab.config.gitlab.url}/foo.png)"
           },
+          'absolute upload URL' => {
+            input: '![an image](/uploads/foo.png)',
+            output: "![an image](#{Gitlab.config.gitlab.url}/baz/bar/uploads/foo.png)"
+          },
+          'absolute non-upload URL' => {
+            input: '![an image](/downloads/foo.png)',
+            output: "![an image](#{Gitlab.config.gitlab.url}/downloads/foo.png)"
+          }
+        }
+      end
+
+      with_them do
+        it { expect(subject.absolute_image_urls(input)).to eq(output) }
+      end
+    end
+
+    context 'without an upload prefix specified' do
+      subject { subclass.new(nil) }
+
+      where do
+        {
+          'relative image URL' => {
+            input: '![an image](foo.png)',
+            output: "![an image](#{Gitlab.config.gitlab.url}/foo.png)"
+          },
+          'absolute upload URL' => {
+            input: '![an image](/uploads/foo.png)',
+            output: "![an image](#{Gitlab.config.gitlab.url}/uploads/foo.png)"
+          },
+          'absolute non-upload URL' => {
+            input: '![an image](/downloads/foo.png)',
+            output: "![an image](#{Gitlab.config.gitlab.url}/downloads/foo.png)"
+          },
           'HTTP URL' => {
             input: '![an image](http://example.com/foo.png)',
             output: '![an image](http://example.com/foo.png)'
@@ -61,4 +97,5 @@ describe Gitlab::HookData::BaseBuilder do
         it { expect(subject.absolute_image_urls(input)).to eq(output) }
       end
     end
+  end
 end

spec/lib/gitlab/hook_data/issue_builder_spec.rb

@@ -46,7 +46,10 @@ describe Gitlab::HookData::IssueBuilder do
       let(:builder) { described_class.new(issue_with_description) }
 
       it 'sets the image to use an absolute URL' do
-        expect(data[:description]).to eq("test![Issue_Image](#{Settings.gitlab.url}/uploads/abc/Issue_Image.png)")
+        expected_path = "#{issue_with_description.project.path_with_namespace}/uploads/abc/Issue_Image.png"
+
+        expect(data[:description])
+          .to eq("test![Issue_Image](#{Settings.gitlab.url}/#{expected_path})")
       end
     end
   end

spec/lib/gitlab/hook_data/merge_request_builder_spec.rb

@@ -58,11 +58,14 @@ describe Gitlab::HookData::MergeRequestBuilder do
     end
 
     context 'when the MR has an image in the description' do
-      let(:mr_with_description) { create(:merge_request, description: 'test![Issue_Image](/uploads/abc/Issue_Image.png)') }
+      let(:mr_with_description) { create(:merge_request, description: 'test![MR_Image](/uploads/abc/MR_Image.png)') }
       let(:builder) { described_class.new(mr_with_description) }
 
       it 'sets the image to use an absolute URL' do
-        expect(data[:description]).to eq("test![Issue_Image](#{Settings.gitlab.url}/uploads/abc/Issue_Image.png)")
+        expected_path = "#{mr_with_description.project.path_with_namespace}/uploads/abc/MR_Image.png"
+
+        expect(data[:description])
+          .to eq("test![MR_Image](#{Settings.gitlab.url}/#{expected_path})")
       end
     end
   end