From 9170aab92e616a6f6d3ddfc4cf8326cba0e4a1a8 Mon Sep 17 00:00:00 2001 From: Satish Perala Date: Thu, 25 Aug 2016 22:35:59 +0530 Subject: [PATCH 01/11] Passing absolute image urls in the markdown content in the webhooks --- app/models/note.rb | 4 +++- app/models/wiki_page.rb | 4 +++- lib/gitlab/hook_data/issue_builder.rb | 1 + lib/gitlab/hook_data/merge_request_builder.rb | 1 + lib/markdown_utils.rb | 8 ++++++++ spec/lib/gitlab/hook_data/issue_builder_spec.rb | 9 +++++++++ spec/models/merge_request_spec.rb | 8 ++++++++ spec/models/note_spec.rb | 8 ++++++++ spec/models/wiki_page_spec.rb | 12 ++++++++++++ 9 files changed, 53 insertions(+), 2 deletions(-) create mode 100644 lib/markdown_utils.rb diff --git a/app/models/note.rb b/app/models/note.rb index 41c04ae0571..3893fe25fb7 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -202,7 +202,9 @@ class Note < ActiveRecord::Base end def hook_attrs - attributes + attributes.merge({ + "note" => MarkdownUtils.absolute_image_urls(self.note) + }) end def for_commit? diff --git a/app/models/wiki_page.rb b/app/models/wiki_page.rb index cde79b95062..5f8552212ad 100644 --- a/app/models/wiki_page.rb +++ b/app/models/wiki_page.rb @@ -59,7 +59,9 @@ class WikiPage attr_accessor :attributes def hook_attrs - attributes + attributes.merge({ + "content" => MarkdownUtils.absolute_image_urls(self.content) + }) end def initialize(wiki, page = nil, persisted = false) diff --git a/lib/gitlab/hook_data/issue_builder.rb b/lib/gitlab/hook_data/issue_builder.rb index f9b1a3caf5e..55dc84a60d4 100644 --- a/lib/gitlab/hook_data/issue_builder.rb +++ b/lib/gitlab/hook_data/issue_builder.rb @@ -38,6 +38,7 @@ module Gitlab def build attrs = { + description: MarkdownUtils.absolute_image_urls(issue.description), url: Gitlab::UrlBuilder.build(issue), total_time_spent: issue.total_time_spent, human_total_time_spent: issue.human_total_time_spent, diff --git a/lib/gitlab/hook_data/merge_request_builder.rb b/lib/gitlab/hook_data/merge_request_builder.rb index aff786864f2..2dfc0435848 100644 --- a/lib/gitlab/hook_data/merge_request_builder.rb +++ b/lib/gitlab/hook_data/merge_request_builder.rb @@ -43,6 +43,7 @@ module Gitlab def build attrs = { + description: MarkdownUtils.absolute_image_urls(issue.description), url: Gitlab::UrlBuilder.build(merge_request), source: merge_request.source_project.try(:hook_attrs), target: merge_request.target_project.hook_attrs, diff --git a/lib/markdown_utils.rb b/lib/markdown_utils.rb new file mode 100644 index 00000000000..a5cf035018b --- /dev/null +++ b/lib/markdown_utils.rb @@ -0,0 +1,8 @@ +# Class to have all utility functions related to markdown +class MarkdownUtils + + # Convert image urls in the markdown text to absolute urls + def self.absolute_image_urls(markdown_text) + markdown_text.gsub(/!\[(.*?)\]\((.*?)\)/, "![\\1](#{Settings.gitlab.url}\\2)") + end +end diff --git a/spec/lib/gitlab/hook_data/issue_builder_spec.rb b/spec/lib/gitlab/hook_data/issue_builder_spec.rb index 506b2c0be20..0e4a1d345f4 100644 --- a/spec/lib/gitlab/hook_data/issue_builder_spec.rb +++ b/spec/lib/gitlab/hook_data/issue_builder_spec.rb @@ -40,5 +40,14 @@ describe Gitlab::HookData::IssueBuilder do expect(data).to include(:human_total_time_spent) expect(data).to include(:assignee_ids) end + + context 'when the issue has an image in the description' do + let(:issue_with_description) { create(:issue, description: 'test![Issue_Image](/uploads/abc/Issue_Image.png)') } + let(:builder) { described_class.new(issue_with_description) } + + it 'adds absolute urls for images in the description' do + expect(data[:description]).to eq("test![Issue_Image](#{Settings.gitlab.url}/uploads/abc/Issue_Image.png)") + end + end end end diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 3f028b3bd5c..da302b5457e 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -2357,4 +2357,12 @@ describe MergeRequest do end end end + + describe '#hook_attrs' do + let(:mr_with_description) { create(:merge_request, description: 'test![Mr_Image](/uploads/abc/Mr_Image.png)') } + + it 'adds absolute urls for images in the description' do + expect(mr_with_description.hook_attrs['description']).to eq("test![Mr_Image](#{Settings.gitlab.url}/uploads/abc/Mr_Image.png)") + end + end end diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb index 6a6c71e6c82..41d84ded1b2 100644 --- a/spec/models/note_spec.rb +++ b/spec/models/note_spec.rb @@ -829,4 +829,12 @@ describe Note do note.destroy! end end + + describe '#hook_attrs' do + let(:note) { create(:note, note: 'test![Note_Image](/uploads/abc/Note_Image.png)') } + + it 'adds absolute urls for images in the description' do + expect(note.hook_attrs['note']).to eq("test![Note_Image](#{Settings.gitlab.url}/uploads/abc/Note_Image.png)") + end + end end diff --git a/spec/models/wiki_page_spec.rb b/spec/models/wiki_page_spec.rb index 1c765ceac2f..c00c628b172 100644 --- a/spec/models/wiki_page_spec.rb +++ b/spec/models/wiki_page_spec.rb @@ -554,6 +554,18 @@ describe WikiPage do end end + describe '#hook_attrs' do + before do + create_page("test page", "test![WikiPage_Image](/uploads/abc/WikiPage_Image.png)") + @page = wiki.wiki.paged("test page") + @wiki_page = WikiPage.new(wiki, @page, true) + end + + it 'adds absolute urls for images in the content' do + expect(@wiki_page.hook_attrs['content']).to eq("test![WikiPage_Image](#{Settings.gitlab.url}/uploads/abc/WikiPage_Image.png)") + end + end + private def remove_temp_repo(path) From a165a4452eb7430715d672191411c76de7e7f659 Mon Sep 17 00:00:00 2001 From: Satish Perala Date: Fri, 26 Aug 2016 09:15:09 +0530 Subject: [PATCH 02/11] Checking for the presense of markdown text --- lib/markdown_utils.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/markdown_utils.rb b/lib/markdown_utils.rb index a5cf035018b..959eda12115 100644 --- a/lib/markdown_utils.rb +++ b/lib/markdown_utils.rb @@ -3,6 +3,10 @@ class MarkdownUtils # Convert image urls in the markdown text to absolute urls def self.absolute_image_urls(markdown_text) - markdown_text.gsub(/!\[(.*?)\]\((.*?)\)/, "![\\1](#{Settings.gitlab.url}\\2)") + if markdown_text.present? + markdown_text.gsub(/!\[(.*?)\]\((.*?)\)/, "![\\1](#{Settings.gitlab.url}\\2)") + else + markdown_text + end end end From 56b432bd998ad866618dc2a41bbd730371e47dde Mon Sep 17 00:00:00 2001 From: Satish Perala Date: Fri, 26 Aug 2016 09:36:03 +0530 Subject: [PATCH 03/11] Removing the empty line at the class body beginning --- lib/markdown_utils.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/markdown_utils.rb b/lib/markdown_utils.rb index 959eda12115..f33d80da14c 100644 --- a/lib/markdown_utils.rb +++ b/lib/markdown_utils.rb @@ -1,6 +1,5 @@ # Class to have all utility functions related to markdown class MarkdownUtils - # Convert image urls in the markdown text to absolute urls def self.absolute_image_urls(markdown_text) if markdown_text.present? From 95265c08216e380a3cd2208ff99956fcdccd5a86 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Tue, 10 Apr 2018 11:35:06 +0100 Subject: [PATCH 04/11] Fix MR hook builder --- lib/gitlab/hook_data/merge_request_builder.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/gitlab/hook_data/merge_request_builder.rb b/lib/gitlab/hook_data/merge_request_builder.rb index 2dfc0435848..6e5ef09de9e 100644 --- a/lib/gitlab/hook_data/merge_request_builder.rb +++ b/lib/gitlab/hook_data/merge_request_builder.rb @@ -43,7 +43,7 @@ module Gitlab def build attrs = { - description: MarkdownUtils.absolute_image_urls(issue.description), + description: MarkdownUtils.absolute_image_urls(merge_request.description), url: Gitlab::UrlBuilder.build(merge_request), source: merge_request.source_project.try(:hook_attrs), target: merge_request.target_project.hook_attrs, From 6e4d67e099cfe15bc5daf5014fd74d9ceb1cb3f3 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Tue, 10 Apr 2018 13:55:51 +0100 Subject: [PATCH 05/11] Fix hook data specs --- spec/lib/gitlab/hook_data/issue_builder_spec.rb | 2 +- spec/lib/gitlab/hook_data/merge_request_builder_spec.rb | 9 +++++++++ spec/models/merge_request_spec.rb | 8 -------- spec/models/wiki_page_spec.rb | 4 ++-- 4 files changed, 12 insertions(+), 11 deletions(-) diff --git a/spec/lib/gitlab/hook_data/issue_builder_spec.rb b/spec/lib/gitlab/hook_data/issue_builder_spec.rb index 0e4a1d345f4..60093474f8a 100644 --- a/spec/lib/gitlab/hook_data/issue_builder_spec.rb +++ b/spec/lib/gitlab/hook_data/issue_builder_spec.rb @@ -45,7 +45,7 @@ describe Gitlab::HookData::IssueBuilder do let(:issue_with_description) { create(:issue, description: 'test![Issue_Image](/uploads/abc/Issue_Image.png)') } let(:builder) { described_class.new(issue_with_description) } - it 'adds absolute urls for images in the description' 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)") end end diff --git a/spec/lib/gitlab/hook_data/merge_request_builder_spec.rb b/spec/lib/gitlab/hook_data/merge_request_builder_spec.rb index b61614e4790..dd586af6118 100644 --- a/spec/lib/gitlab/hook_data/merge_request_builder_spec.rb +++ b/spec/lib/gitlab/hook_data/merge_request_builder_spec.rb @@ -56,5 +56,14 @@ describe Gitlab::HookData::MergeRequestBuilder do expect(data).to include(:human_time_estimate) expect(data).to include(:human_total_time_spent) 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(: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)") + end + end end end diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index da302b5457e..3f028b3bd5c 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -2357,12 +2357,4 @@ describe MergeRequest do end end end - - describe '#hook_attrs' do - let(:mr_with_description) { create(:merge_request, description: 'test![Mr_Image](/uploads/abc/Mr_Image.png)') } - - it 'adds absolute urls for images in the description' do - expect(mr_with_description.hook_attrs['description']).to eq("test![Mr_Image](#{Settings.gitlab.url}/uploads/abc/Mr_Image.png)") - end - end end diff --git a/spec/models/wiki_page_spec.rb b/spec/models/wiki_page_spec.rb index c00c628b172..4da6a423d15 100644 --- a/spec/models/wiki_page_spec.rb +++ b/spec/models/wiki_page_spec.rb @@ -557,8 +557,8 @@ describe WikiPage do describe '#hook_attrs' do before do create_page("test page", "test![WikiPage_Image](/uploads/abc/WikiPage_Image.png)") - @page = wiki.wiki.paged("test page") - @wiki_page = WikiPage.new(wiki, @page, true) + @page = wiki.wiki.page(title: "test page") + @wiki_page = described_class.new(wiki, @page, true) end it 'adds absolute urls for images in the content' do From 7ff24772b63691f857a1ee1d113dcbdc3b01f064 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Wed, 20 Jun 2018 14:53:19 +0100 Subject: [PATCH 06/11] Add base class for hook builders, and use it for notes and wikis --- app/models/note.rb | 4 +- app/models/wiki_page.rb | 4 +- lib/gitlab/hook_data/base_builder.rb | 20 +++++++++ lib/gitlab/hook_data/issuable_builder.rb | 15 ++++--- lib/gitlab/hook_data/issue_builder.rb | 10 ++--- lib/gitlab/hook_data/merge_request_builder.rb | 10 ++--- lib/gitlab/hook_data/note_builder.rb | 43 +++++++++++++++++++ lib/gitlab/hook_data/wiki_page_builder.rb | 15 +++++++ lib/markdown_utils.rb | 11 ----- spec/models/note_spec.rb | 8 ---- 10 files changed, 95 insertions(+), 45 deletions(-) create mode 100644 lib/gitlab/hook_data/base_builder.rb create mode 100644 lib/gitlab/hook_data/note_builder.rb create mode 100644 lib/gitlab/hook_data/wiki_page_builder.rb delete mode 100644 lib/markdown_utils.rb diff --git a/app/models/note.rb b/app/models/note.rb index 3893fe25fb7..b407d3c18ad 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -202,9 +202,7 @@ class Note < ActiveRecord::Base end def hook_attrs - attributes.merge({ - "note" => MarkdownUtils.absolute_image_urls(self.note) - }) + Gitlab::HookData::NoteBuilder.new(self).build end def for_commit? diff --git a/app/models/wiki_page.rb b/app/models/wiki_page.rb index 5f8552212ad..fc97ebe9f63 100644 --- a/app/models/wiki_page.rb +++ b/app/models/wiki_page.rb @@ -59,9 +59,7 @@ class WikiPage attr_accessor :attributes def hook_attrs - attributes.merge({ - "content" => MarkdownUtils.absolute_image_urls(self.content) - }) + Gitlab::HookData::WikiPageBuilder.new(self).build end def initialize(wiki, page = nil, persisted = false) diff --git a/lib/gitlab/hook_data/base_builder.rb b/lib/gitlab/hook_data/base_builder.rb new file mode 100644 index 00000000000..1d99e793a03 --- /dev/null +++ b/lib/gitlab/hook_data/base_builder.rb @@ -0,0 +1,20 @@ +module Gitlab + module HookData + class BaseBuilder + attr_accessor :object + + def initialize(object) + @object = object + end + + private + + def absolute_image_urls(markdown_text) + return markdown_text unless markdown_text.present? + + markdown_text.gsub(/!\[(.*?)\]\((.*?)\)/, + "![\\1](#{Settings.gitlab.url}\\2)") + end + end + end +end diff --git a/lib/gitlab/hook_data/issuable_builder.rb b/lib/gitlab/hook_data/issuable_builder.rb index 6ab36676127..ad0d6f6352b 100644 --- a/lib/gitlab/hook_data/issuable_builder.rb +++ b/lib/gitlab/hook_data/issuable_builder.rb @@ -1,13 +1,9 @@ module Gitlab module HookData - class IssuableBuilder + class IssuableBuilder < BaseBuilder CHANGES_KEYS = %i[previous current].freeze - attr_accessor :issuable - - def initialize(issuable) - @issuable = issuable - end + alias_method :issuable, :object def build(user: nil, changes: {}) hook_data = { @@ -64,6 +60,13 @@ module Gitlab hash end end + + def absolute_image_urls(markdown_text) + return markdown_text unless markdown_text.present? + + markdown_text.gsub(/!\[(.*?)\]\((.*?)\)/, + "![\\1](#{Settings.gitlab.url}\\2)") + end end end end diff --git a/lib/gitlab/hook_data/issue_builder.rb b/lib/gitlab/hook_data/issue_builder.rb index 55dc84a60d4..0d71c748dc6 100644 --- a/lib/gitlab/hook_data/issue_builder.rb +++ b/lib/gitlab/hook_data/issue_builder.rb @@ -1,6 +1,6 @@ module Gitlab module HookData - class IssueBuilder + class IssueBuilder < BaseBuilder SAFE_HOOK_ATTRIBUTES = %i[ assignee_id author_id @@ -30,15 +30,11 @@ module Gitlab total_time_spent ].freeze - attr_accessor :issue - - def initialize(issue) - @issue = issue - end + alias_method :issue, :object def build attrs = { - description: MarkdownUtils.absolute_image_urls(issue.description), + description: absolute_image_urls(issue.description), url: Gitlab::UrlBuilder.build(issue), total_time_spent: issue.total_time_spent, human_total_time_spent: issue.human_total_time_spent, diff --git a/lib/gitlab/hook_data/merge_request_builder.rb b/lib/gitlab/hook_data/merge_request_builder.rb index 6e5ef09de9e..dfbed0597ed 100644 --- a/lib/gitlab/hook_data/merge_request_builder.rb +++ b/lib/gitlab/hook_data/merge_request_builder.rb @@ -1,6 +1,6 @@ module Gitlab module HookData - class MergeRequestBuilder + class MergeRequestBuilder < BaseBuilder SAFE_HOOK_ATTRIBUTES = %i[ assignee_id author_id @@ -35,15 +35,11 @@ module Gitlab total_time_spent ].freeze - attr_accessor :merge_request - - def initialize(merge_request) - @merge_request = merge_request - end + alias_method :merge_request, :object def build attrs = { - description: MarkdownUtils.absolute_image_urls(merge_request.description), + description: absolute_image_urls(merge_request.description), url: Gitlab::UrlBuilder.build(merge_request), source: merge_request.source_project.try(:hook_attrs), target: merge_request.target_project.hook_attrs, diff --git a/lib/gitlab/hook_data/note_builder.rb b/lib/gitlab/hook_data/note_builder.rb new file mode 100644 index 00000000000..81873e345d5 --- /dev/null +++ b/lib/gitlab/hook_data/note_builder.rb @@ -0,0 +1,43 @@ +module Gitlab + module HookData + class NoteBuilder < BaseBuilder + SAFE_HOOK_ATTRIBUTES = %i[ + attachment + author_id + change_position + commit_id + created_at + discussion_id + id + line_code + note + noteable_id + noteable_type + original_position + position + project_id + resolved_at + resolved_by_id + resolved_by_push + st_diff + system + type + updated_at + updated_by_id + ].freeze + + alias_method :note, :object + + def build + note + .attributes + .with_indifferent_access + .slice(*SAFE_HOOK_ATTRIBUTES) + .merge( + description: absolute_image_urls(note.note), + url: Gitlab::UrlBuilder.build(note) + ) + end + end + end +end diff --git a/lib/gitlab/hook_data/wiki_page_builder.rb b/lib/gitlab/hook_data/wiki_page_builder.rb new file mode 100644 index 00000000000..59c94a61cf2 --- /dev/null +++ b/lib/gitlab/hook_data/wiki_page_builder.rb @@ -0,0 +1,15 @@ +module Gitlab + module HookData + class WikiPageBuilder < BaseBuilder + alias_method :wiki_page, :object + + def build + wiki_page + .attributes + .merge( + 'content' => absolute_image_urls(wiki_page.content) + ) + end + end + end +end diff --git a/lib/markdown_utils.rb b/lib/markdown_utils.rb deleted file mode 100644 index f33d80da14c..00000000000 --- a/lib/markdown_utils.rb +++ /dev/null @@ -1,11 +0,0 @@ -# Class to have all utility functions related to markdown -class MarkdownUtils - # Convert image urls in the markdown text to absolute urls - def self.absolute_image_urls(markdown_text) - if markdown_text.present? - markdown_text.gsub(/!\[(.*?)\]\((.*?)\)/, "![\\1](#{Settings.gitlab.url}\\2)") - else - markdown_text - end - end -end diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb index 41d84ded1b2..6a6c71e6c82 100644 --- a/spec/models/note_spec.rb +++ b/spec/models/note_spec.rb @@ -829,12 +829,4 @@ describe Note do note.destroy! end end - - describe '#hook_attrs' do - let(:note) { create(:note, note: 'test![Note_Image](/uploads/abc/Note_Image.png)') } - - it 'adds absolute urls for images in the description' do - expect(note.hook_attrs['note']).to eq("test![Note_Image](#{Settings.gitlab.url}/uploads/abc/Note_Image.png)") - end - end end From 0d9ef34a2541a2adf00677132eac3637de33b6d4 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Fri, 6 Jul 2018 11:35:30 +0100 Subject: [PATCH 07/11] Add documentation and specs for webhook URL rewriting --- ...itlab-ce-20720_webhooks_full_image_url.yml | 5 ++ doc/user/project/integrations/webhooks.md | 28 ++++++++++ lib/banzai/filter/blockquote_fence_filter.rb | 22 +------- lib/gitlab/hook_data/base_builder.rb | 27 +++++++-- lib/gitlab/hook_data/issuable_builder.rb | 7 --- lib/gitlab/regex.rb | 26 +++++++++ .../lib/gitlab/hook_data/base_builder_spec.rb | 56 +++++++++++++++++++ 7 files changed, 139 insertions(+), 32 deletions(-) create mode 100644 changelogs/unreleased/satishperala-gitlab-ce-20720_webhooks_full_image_url.yml create mode 100644 spec/lib/gitlab/hook_data/base_builder_spec.rb diff --git a/changelogs/unreleased/satishperala-gitlab-ce-20720_webhooks_full_image_url.yml b/changelogs/unreleased/satishperala-gitlab-ce-20720_webhooks_full_image_url.yml new file mode 100644 index 00000000000..7bfe1b5778f --- /dev/null +++ b/changelogs/unreleased/satishperala-gitlab-ce-20720_webhooks_full_image_url.yml @@ -0,0 +1,5 @@ +--- +title: Include full image URL in webhooks for uploaded images +merge_request: 18109 +author: Satish Perala +type: changed diff --git a/doc/user/project/integrations/webhooks.md b/doc/user/project/integrations/webhooks.md index 19df78f4140..5f33e0c1dc6 100644 --- a/doc/user/project/integrations/webhooks.md +++ b/doc/user/project/integrations/webhooks.md @@ -6,6 +6,13 @@ Starting from GitLab 8.5: - the `project.ssh_url` key is deprecated in favor of the `project.git_ssh_url` key - the `project.http_url` key is deprecated in favor of the `project.git_http_url` key +>**Note** +Starting from GitLab 11.2: +- The `description` field for issues, merge requests, comments, and wiki pages + is rewritten so that simple Markdown image references (like + `![](/uploads/...)`) have their target URL changed to an absolute URL. See + [image URL rewriting](#image-url-rewriting) for more details. + Project webhooks allow you to trigger a URL if for example new code is pushed or a new issue is created. You can configure webhooks to listen for specific events like pushes, issues or merge requests. GitLab will send a POST request with data @@ -1121,6 +1128,27 @@ X-Gitlab-Event: Build Hook } ``` +## Image URL rewriting + +From GitLab 11.2, simple image references are rewritten to use an absolute URL +in webhooks. So if an image, merge request, comment, or wiki page has this in +its description: + +```markdown +![image](/uploads/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa/image.png) +``` + +It will appear in the webhook body as the below (assuming that GitLab is +installed at gitlab.example.com): + +```markdown +![image](https://gitlab.example.com/uploads/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa/image.png) +``` + +This will not rewrite URLs that already are pointing to HTTP, HTTPS, or +protocol-relative URLs. It will also not rewrite image URLs using advanced +Markdown features, like link labels. + ## Testing webhooks You can trigger the webhook manually. Sample data from the project will be used.Sample data will take from the project. diff --git a/lib/banzai/filter/blockquote_fence_filter.rb b/lib/banzai/filter/blockquote_fence_filter.rb index fbfcd72c916..7108e828c6d 100644 --- a/lib/banzai/filter/blockquote_fence_filter.rb +++ b/lib/banzai/filter/blockquote_fence_filter.rb @@ -2,27 +2,7 @@ module Banzai module Filter class BlockquoteFenceFilter < HTML::Pipeline::TextFilter REGEX = %r{ - (? - # Code blocks: - # ``` - # Anything, including `>>>` blocks which are ignored by this filter - # ``` - - ^``` - .+? - \n```\ *$ - ) - | - (? - # HTML block: - # - # Anything, including `>>>` blocks which are ignored by this filter - # - - ^<[^>]+?>\ *\n - .+? - \n<\/[^>]+?>\ *$ - ) + #{::Gitlab::Regex.markdown_code_or_html_blocks} | (?: # Blockquote: diff --git a/lib/gitlab/hook_data/base_builder.rb b/lib/gitlab/hook_data/base_builder.rb index 1d99e793a03..da0f4e6105e 100644 --- a/lib/gitlab/hook_data/base_builder.rb +++ b/lib/gitlab/hook_data/base_builder.rb @@ -3,17 +3,36 @@ module Gitlab class BaseBuilder attr_accessor :object + MARKDOWN_SIMPLE_IMAGE = %r{ + #{::Gitlab::Regex.markdown_code_or_html_blocks} + | + (? + ! + \[(?[^\n]*?)\] + \((?<url>(?!(https?://|//))[^\n]+?)\) + ) + }mx.freeze + def initialize(object) @object = object end + def self.absolute_image_urls(markdown_text) + return markdown_text unless markdown_text.present? + + markdown_text.gsub(MARKDOWN_SIMPLE_IMAGE) do + if $~[:image] + "![#{$~[:title]}](#{Gitlab.config.gitlab.url}/#{$~[:url]})" + else + $~[0] + end + end + end + private def absolute_image_urls(markdown_text) - return markdown_text unless markdown_text.present? - - markdown_text.gsub(/!\[(.*?)\]\((.*?)\)/, - "![\\1](#{Settings.gitlab.url}\\2)") + self.class.absolute_image_urls(markdown_text) end end end diff --git a/lib/gitlab/hook_data/issuable_builder.rb b/lib/gitlab/hook_data/issuable_builder.rb index ad0d6f6352b..f2eda398b8f 100644 --- a/lib/gitlab/hook_data/issuable_builder.rb +++ b/lib/gitlab/hook_data/issuable_builder.rb @@ -60,13 +60,6 @@ module Gitlab hash end end - - def absolute_image_urls(markdown_text) - return markdown_text unless markdown_text.present? - - markdown_text.gsub(/!\[(.*?)\]\((.*?)\)/, - "![\\1](#{Settings.gitlab.url}\\2)") - end end end end diff --git a/lib/gitlab/regex.rb b/lib/gitlab/regex.rb index ac3de2a8f71..e1a958c508a 100644 --- a/lib/gitlab/regex.rb +++ b/lib/gitlab/regex.rb @@ -73,5 +73,31 @@ module Gitlab def build_trace_section_regex @build_trace_section_regexp ||= /section_((?:start)|(?:end)):(\d+):([a-zA-Z0-9_.-]+)\r\033\[0K/.freeze end + + def markdown_code_or_html_blocks + @markdown_code_or_html_blocks ||= %r{ + (?<code> + # Code blocks: + # ``` + # Anything, including `>>>` blocks which are ignored by this filter + # ``` + + ^``` + .+? + \n```\ *$ + ) + | + (?<html> + # HTML block: + # <tag> + # Anything, including `>>>` blocks which are ignored by this filter + # </tag> + + ^<[^>]+?>\ *\n + .+? + \n<\/[^>]+?>\ *$ + ) + }mx + end end end diff --git a/spec/lib/gitlab/hook_data/base_builder_spec.rb b/spec/lib/gitlab/hook_data/base_builder_spec.rb new file mode 100644 index 00000000000..c5cc6177b1e --- /dev/null +++ b/spec/lib/gitlab/hook_data/base_builder_spec.rb @@ -0,0 +1,56 @@ +require 'spec_helper' + +describe Gitlab::HookData::BaseBuilder do + describe '.absolute_image_urls' do + using RSpec::Parameterized::TableSyntax + + where do + { + 'relative image URL' => { + input: '![an image](foo.png)', + output: "![an image](#{Gitlab.config.gitlab.url}/foo.png)" + }, + 'HTTP URL' => { + input: '![an image](http://example.com/foo.png)', + output: '![an image](http://example.com/foo.png)' + }, + 'HTTPS URL' => { + input: '![an image](https://example.com/foo.png)', + output: '![an image](https://example.com/foo.png)' + }, + 'protocol-relative URL' => { + input: '![an image](//example.com/foo.png)', + output: '![an image](//example.com/foo.png)' + }, + 'URL reference by title' => { + input: "![foo]\n\n[foo]: foo.png", + output: "![foo]\n\n[foo]: foo.png" + }, + 'URL reference by label' => { + input: "![][foo]\n\n[foo]: foo.png", + output: "![][foo]\n\n[foo]: foo.png" + }, + 'in Markdown inline code block' => { + input: '`![an image](foo.png)`', + output: "`![an image](#{Gitlab.config.gitlab.url}/foo.png)`" + }, + 'in HTML tag on the same line' => { + input: '<p>![an image](foo.png)</p>', + output: "<p>![an image](#{Gitlab.config.gitlab.url}/foo.png)</p>" + }, + 'in Markdown multi-line code block' => { + input: "```\n![an image](foo.png)\n```", + output: "```\n![an image](foo.png)\n```" + }, + 'in HTML tag on different lines' => { + input: "<p>\n![an image](foo.png)\n</p>", + output: "<p>\n![an image](foo.png)\n</p>" + } + } + end + + with_them do + it { expect(described_class.absolute_image_urls(input)).to eq(output) } + end + end +end From 25664f89acc1f25228e97a5169f0e7255124df2a Mon Sep 17 00:00:00 2001 From: Sean McGivern <sean@gitlab.com> Date: Fri, 6 Jul 2018 13:44:45 +0100 Subject: [PATCH 08/11] Don't include a double slash when rewriting the URL --- lib/gitlab/hook_data/base_builder.rb | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/gitlab/hook_data/base_builder.rb b/lib/gitlab/hook_data/base_builder.rb index da0f4e6105e..1e90a2b3fe1 100644 --- a/lib/gitlab/hook_data/base_builder.rb +++ b/lib/gitlab/hook_data/base_builder.rb @@ -22,7 +22,10 @@ module Gitlab markdown_text.gsub(MARKDOWN_SIMPLE_IMAGE) do if $~[:image] - "![#{$~[:title]}](#{Gitlab.config.gitlab.url}/#{$~[:url]})" + url = $~[:url] + url = "/#{url}" unless url.start_with?('/') + + "![#{$~[:title]}](#{Gitlab.config.gitlab.url}#{url})" else $~[0] end From 35d2a058e574546fadb2ccf85132a777b20360c1 Mon Sep 17 00:00:00 2001 From: Sean McGivern <sean@gitlab.com> Date: Tue, 17 Jul 2018 10:37:37 +0100 Subject: [PATCH 09/11] Clarify web hook image rewriting doc example --- doc/user/project/integrations/webhooks.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/doc/user/project/integrations/webhooks.md b/doc/user/project/integrations/webhooks.md index 9bd4dbd7631..8e486318980 100644 --- a/doc/user/project/integrations/webhooks.md +++ b/doc/user/project/integrations/webhooks.md @@ -1139,14 +1139,14 @@ in webhooks. So if an image, merge request, comment, or wiki page has this in its description: ```markdown -![image](/uploads/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa/image.png) +![image](/uploads/$sha/image.png) ``` It will appear in the webhook body as the below (assuming that GitLab is installed at gitlab.example.com): ```markdown -![image](https://gitlab.example.com/uploads/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa/image.png) +![image](https://gitlab.example.com/uploads/$sha/image.png) ``` This will not rewrite URLs that already are pointing to HTTP, HTTPS, or From 1e2c28ab4194217ffcacd4e2e6cd41b1cc50bb09 Mon Sep 17 00:00:00 2001 From: Sean McGivern <sean@gitlab.com> Date: Tue, 17 Jul 2018 10:38:09 +0100 Subject: [PATCH 10/11] Simplify WikiPage#hook_attrs spec --- spec/models/wiki_page_spec.rb | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/spec/models/wiki_page_spec.rb b/spec/models/wiki_page_spec.rb index 4da6a423d15..63850939be1 100644 --- a/spec/models/wiki_page_spec.rb +++ b/spec/models/wiki_page_spec.rb @@ -555,14 +555,12 @@ describe WikiPage do end describe '#hook_attrs' do - before do - create_page("test page", "test![WikiPage_Image](/uploads/abc/WikiPage_Image.png)") - @page = wiki.wiki.page(title: "test page") - @wiki_page = described_class.new(wiki, @page, true) - end - it 'adds absolute urls for images in the content' do - expect(@wiki_page.hook_attrs['content']).to eq("test![WikiPage_Image](#{Settings.gitlab.url}/uploads/abc/WikiPage_Image.png)") + create_page("test page", "test![WikiPage_Image](/uploads/abc/WikiPage_Image.png)") + page = wiki.wiki.page(title: "test page") + wiki_page = described_class.new(wiki, page, true) + + expect(wiki_page.hook_attrs['content']).to eq("test![WikiPage_Image](#{Settings.gitlab.url}/uploads/abc/WikiPage_Image.png)") end end From fc580935ca2171d4f5628818aa4826fbce4a7261 Mon Sep 17 00:00:00 2001 From: Sean McGivern <sean@gitlab.com> Date: Tue, 17 Jul 2018 14:58:14 +0100 Subject: [PATCH 11/11] Keep #absolute_image_urls as a private instance method --- lib/gitlab/hook_data/base_builder.rb | 10 +++------- spec/lib/gitlab/hook_data/base_builder_spec.rb | 12 ++++++++++-- 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/lib/gitlab/hook_data/base_builder.rb b/lib/gitlab/hook_data/base_builder.rb index 1e90a2b3fe1..4ffca356b29 100644 --- a/lib/gitlab/hook_data/base_builder.rb +++ b/lib/gitlab/hook_data/base_builder.rb @@ -17,7 +17,9 @@ module Gitlab @object = object end - def self.absolute_image_urls(markdown_text) + private + + def absolute_image_urls(markdown_text) return markdown_text unless markdown_text.present? markdown_text.gsub(MARKDOWN_SIMPLE_IMAGE) do @@ -31,12 +33,6 @@ module Gitlab end end end - - private - - def absolute_image_urls(markdown_text) - self.class.absolute_image_urls(markdown_text) - end end end end diff --git a/spec/lib/gitlab/hook_data/base_builder_spec.rb b/spec/lib/gitlab/hook_data/base_builder_spec.rb index c5cc6177b1e..a921dd766c3 100644 --- a/spec/lib/gitlab/hook_data/base_builder_spec.rb +++ b/spec/lib/gitlab/hook_data/base_builder_spec.rb @@ -1,7 +1,15 @@ require 'spec_helper' describe Gitlab::HookData::BaseBuilder do - describe '.absolute_image_urls' do + describe '#absolute_image_urls' do + let(:subclass) do + Class.new(described_class) do + public :absolute_image_urls + end + end + + subject { subclass.new(nil) } + using RSpec::Parameterized::TableSyntax where do @@ -50,7 +58,7 @@ describe Gitlab::HookData::BaseBuilder do end with_them do - it { expect(described_class.absolute_image_urls(input)).to eq(output) } + it { expect(subject.absolute_image_urls(input)).to eq(output) } end end end