Commit 0bcfd0ad authored by Sean McGivern's avatar Sean McGivern

Fix image webhook rewriting for uploads

This rewrote URLs to be absolute URLs. However, for uploads (the most
common case), we actually need them to point to not just the GitLab
instance, but the project they're from. Thankfully, we can normally get
that information from the object we're building the hook for.
parent c7fd9558
---
title: Fix auto-corrected upload URLs in webhooks
merge_request: 22361
author:
type: fixed
...@@ -1157,10 +1157,11 @@ its description: ...@@ -1157,10 +1157,11 @@ its description:
``` ```
It will appear in the webhook body as the below (assuming that GitLab is 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 ```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 This will not rewrite URLs that already are pointing to HTTP, HTTPS, or
......
...@@ -25,6 +25,7 @@ module Gitlab ...@@ -25,6 +25,7 @@ module Gitlab
markdown_text.gsub(MARKDOWN_SIMPLE_IMAGE) do markdown_text.gsub(MARKDOWN_SIMPLE_IMAGE) do
if $~[:image] if $~[:image]
url = $~[:url] url = $~[:url]
url = "#{uploads_prefix}#{url}" if url.start_with?('/uploads')
url = "/#{url}" unless url.start_with?('/') url = "/#{url}" unless url.start_with?('/')
"![#{$~[:title]}](#{Gitlab.config.gitlab.url}#{url})" "![#{$~[:title]}](#{Gitlab.config.gitlab.url}#{url})"
...@@ -33,6 +34,16 @@ module Gitlab ...@@ -33,6 +34,16 @@ module Gitlab
end end
end end
end end
def uploads_prefix
project&.full_path || ''
end
def project
return unless object.respond_to?(:project)
object.project
end
end end
end end
end end
...@@ -8,16 +8,52 @@ describe Gitlab::HookData::BaseBuilder do ...@@ -8,16 +8,52 @@ describe Gitlab::HookData::BaseBuilder do
end end
end end
subject { subclass.new(nil) }
using RSpec::Parameterized::TableSyntax 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 where do
{ {
'relative image URL' => { 'relative image URL' => {
input: '![an image](foo.png)', input: '![an image](foo.png)',
output: "![an image](#{Gitlab.config.gitlab.url}/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' => { 'HTTP URL' => {
input: '![an image](http://example.com/foo.png)', input: '![an image](http://example.com/foo.png)',
output: '![an image](http://example.com/foo.png)' output: '![an image](http://example.com/foo.png)'
...@@ -61,4 +97,5 @@ describe Gitlab::HookData::BaseBuilder do ...@@ -61,4 +97,5 @@ describe Gitlab::HookData::BaseBuilder do
it { expect(subject.absolute_image_urls(input)).to eq(output) } it { expect(subject.absolute_image_urls(input)).to eq(output) }
end end
end end
end
end end
...@@ -46,7 +46,10 @@ describe Gitlab::HookData::IssueBuilder do ...@@ -46,7 +46,10 @@ describe Gitlab::HookData::IssueBuilder do
let(:builder) { described_class.new(issue_with_description) } let(:builder) { described_class.new(issue_with_description) }
it 'sets the image to use an absolute URL' do 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 end
end end
......
...@@ -58,11 +58,14 @@ describe Gitlab::HookData::MergeRequestBuilder do ...@@ -58,11 +58,14 @@ describe Gitlab::HookData::MergeRequestBuilder do
end end
context 'when the MR has an image in the description' do 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) } let(:builder) { described_class.new(mr_with_description) }
it 'sets the image to use an absolute URL' do 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 end
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment