From 837dc328bf1632e80de4c288a13eb22661177ca5 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Thu, 16 Apr 2015 16:41:51 -0400 Subject: [PATCH 1/5] Change html-pipeline-gitlab gem to html-pipeline --- Gemfile | 2 +- Gemfile.lock | 8 +------- 2 files changed, 2 insertions(+), 8 deletions(-) diff --git a/Gemfile b/Gemfile index 11407ca8e7..3c42885ef8 100644 --- a/Gemfile +++ b/Gemfile @@ -88,7 +88,7 @@ gem "six" gem "seed-fu" # Markup pipeline for GitLab -gem 'html-pipeline-gitlab', '~> 0.1' +gem 'html-pipeline', '~> 1.11.0' # Markdown to HTML gem "github-markup" diff --git a/Gemfile.lock b/Gemfile.lock index a134fbf6bf..8f66476ad6 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -278,12 +278,6 @@ GEM html-pipeline (1.11.0) activesupport (>= 2) nokogiri (~> 1.4) - html-pipeline-gitlab (0.2.0) - actionpack (~> 4) - gitlab_emoji (~> 0.1) - html-pipeline (~> 1.11.0) - mime-types - sanitize (~> 2.1) http_parser.rb (0.5.3) httparty (0.13.3) json (~> 1.8) @@ -717,7 +711,7 @@ DEPENDENCIES guard-spinach haml-rails hipchat (~> 1.5.0) - html-pipeline-gitlab (~> 0.1) + html-pipeline (~> 1.11.0) httparty jasmine (= 2.0.2) jquery-atwho-rails (~> 0.3.3) From 4ced630fed7cc057e66f2c795ebca17a5d6a22e6 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Thu, 16 Apr 2015 17:38:01 -0400 Subject: [PATCH 2/5] Add Gitlab::Markdown::EmojiFilter --- lib/gitlab/markdown/emoji_filter.rb | 79 +++++++++++++++ spec/lib/gitlab/markdown/emoji_filter_spec.rb | 97 +++++++++++++++++++ 2 files changed, 176 insertions(+) create mode 100644 lib/gitlab/markdown/emoji_filter.rb create mode 100644 spec/lib/gitlab/markdown/emoji_filter_spec.rb diff --git a/lib/gitlab/markdown/emoji_filter.rb b/lib/gitlab/markdown/emoji_filter.rb new file mode 100644 index 0000000000..e239f76684 --- /dev/null +++ b/lib/gitlab/markdown/emoji_filter.rb @@ -0,0 +1,79 @@ +require 'gitlab_emoji' +require 'html/pipeline/filter' +require 'action_controller' + +module Gitlab + module Markdown + # HTML filter that replaces :emoji: with images. + # + # Based on HTML::Pipeline::EmojiFilter + # + # Context options: + # :asset_root + # :asset_host + class EmojiFilter < HTML::Pipeline::Filter + IGNORED_ANCESTOR_TAGS = %w(pre code tt).to_set + + def call + doc.search('text()').each do |node| + content = node.to_html + next unless content.include?(':') + next if has_ancestor?(node, IGNORED_ANCESTOR_TAGS) + + html = emoji_image_filter(content) + + next if html == content + + node.replace(html) + end + + doc + end + + # Replace :emoji: with corresponding images. + # + # text - String text to replace :emoji: in. + # + # Returns a String with :emoji: replaced with images. + def emoji_image_filter(text) + text.gsub(emoji_pattern) do |match| + name = $1 + ":#{name}:" + end + end + + private + + def emoji_url(name) + emoji_path = "emoji/#{emoji_filename(name)}" + if context[:asset_host] + # Asset host is specified. + url_to_image(emoji_path) + elsif context[:asset_root] + # Gitlab url is specified + File.join(context[:asset_root], url_to_image(emoji_path)) + else + # All other cases + url_to_image(emoji_path) + end + end + + def url_to_image(image) + ActionController::Base.helpers.url_to_image(image) + end + + # Build a regexp that matches all valid :emoji: names. + def self.emoji_pattern + @emoji_pattern ||= /:(#{Emoji.emojis_names.map { |name| Regexp.escape(name) }.join('|')}):/ + end + + def emoji_pattern + self.class.emoji_pattern + end + + def emoji_filename(name) + "#{Emoji.emoji_filename(name)}.png" + end + end + end +end diff --git a/spec/lib/gitlab/markdown/emoji_filter_spec.rb b/spec/lib/gitlab/markdown/emoji_filter_spec.rb new file mode 100644 index 0000000000..18d55c4818 --- /dev/null +++ b/spec/lib/gitlab/markdown/emoji_filter_spec.rb @@ -0,0 +1,97 @@ +require 'spec_helper' + +module Gitlab::Markdown + describe EmojiFilter do + def filter(html, contexts = {}) + described_class.call(html, contexts) + end + + before do + ActionController::Base.asset_host = 'https://foo.com' + end + + it 'replaces supported emoji' do + doc = filter('

:heart:

') + expect(doc.css('img').first.attr('src')).to eq 'https://foo.com/assets/emoji/2764.png' + end + + it 'ignores unsupported emoji' do + exp = act = '

:foo:

' + doc = filter(act) + expect(doc.to_html).to match Regexp.escape(exp) + end + + it 'correctly encodes the URL' do + doc = filter('

:+1:

') + expect(doc.css('img').first.attr('src')).to eq 'https://foo.com/assets/emoji/1F44D.png' + end + + it 'matches at the start of a string' do + doc = filter(':+1:') + expect(doc.css('img').size).to eq 1 + end + + it 'matches at the end of a string' do + doc = filter('This gets a :-1:') + expect(doc.css('img').size).to eq 1 + end + + it 'matches with adjacent text' do + doc = filter('+1 (:+1:)') + expect(doc.css('img').size).to eq 1 + end + + it 'matches multiple emoji in a row' do + doc = filter(':see_no_evil::hear_no_evil::speak_no_evil:') + expect(doc.css('img').size).to eq 3 + end + + it 'has a title attribute' do + doc = filter(':-1:') + expect(doc.css('img').first.attr('title')).to eq ':-1:' + end + + it 'has an alt attribute' do + doc = filter(':-1:') + expect(doc.css('img').first.attr('alt')).to eq ':-1:' + end + + it 'has an align attribute' do + doc = filter(':8ball:') + expect(doc.css('img').first.attr('align')).to eq 'absmiddle' + end + + it 'has an emoji class' do + doc = filter(':cat:') + expect(doc.css('img').first.attr('class')).to eq 'emoji' + end + + it 'has height and width attributes' do + doc = filter(':dog:') + img = doc.css('img').first + + expect(img.attr('width')).to eq '20' + expect(img.attr('height')).to eq '20' + end + + it 'keeps whitespace intact' do + doc = filter('This deserves a :+1:, big time.') + + expect(doc.to_html).to match(/^This deserves a , big time\.\z/) + end + + it 'uses a custom asset_root context' do + root = Gitlab.config.gitlab.url + 'gitlab/root' + + doc = filter(':smile:', asset_root: root) + expect(doc.css('img').first.attr('src')).to start_with(root) + end + + it 'uses a custom asset_host context' do + ActionController::Base.asset_host = 'https://cdn.example.com' + + doc = filter(':frowning:', asset_host: 'https://this-is-ignored-i-guess?') + expect(doc.css('img').first.attr('src')).to start_with('https://cdn.example.com') + end + end +end From fca624670ac212d4241fa262dbaf1f908e7658b9 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Thu, 16 Apr 2015 17:39:09 -0400 Subject: [PATCH 3/5] Update Gitlab::Markdown to use the :new: EmojiFilter :sparkles: Removes emoji-centric tests from GFM specs :boom: --- lib/gitlab/markdown.rb | 28 ++++---- spec/helpers/gitlab_markdown_helper_spec.rb | 76 +-------------------- 2 files changed, 18 insertions(+), 86 deletions(-) diff --git a/lib/gitlab/markdown.rb b/lib/gitlab/markdown.rb index f230201543..37b250d353 100644 --- a/lib/gitlab/markdown.rb +++ b/lib/gitlab/markdown.rb @@ -1,5 +1,4 @@ require 'html/pipeline' -require 'html/pipeline/gitlab' module Gitlab # Custom parser for GitLab-flavored Markdown @@ -61,19 +60,24 @@ module Gitlab reference_only_path: true ) - markdown_context = { + pipeline = HTML::Pipeline.new(filters) + + context = { + # SanitizationFilter + whitelist: sanitization_whitelist, + + # EmojiFilter asset_root: Gitlab.config.gitlab.url, asset_host: Gitlab::Application.config.asset_host, - whitelist: sanitization_whitelist, - reference_class: html_options[:class], - only_path: options[:reference_only_path], + + # ReferenceFilter current_user: current_user, - project: project + only_path: options[:reference_only_path], + project: project, + reference_class: html_options[:class] } - markdown_pipeline = HTML::Pipeline::Gitlab.new(filters).pipeline - - result = markdown_pipeline.call(text, markdown_context) + result = pipeline.call(text, context) save_options = 0 if options[:xhtml] @@ -91,7 +95,7 @@ module Gitlab private - # Custom filters for html-pipeline: + # Filters used in our pipeline # # SanitizationFilter should come first so that all generated reference HTML # goes through untouched. @@ -101,6 +105,8 @@ module Gitlab [ HTML::Pipeline::SanitizationFilter, + Gitlab::Markdown::EmojiFilter, + Gitlab::Markdown::UserReferenceFilter, Gitlab::Markdown::IssueReferenceFilter, Gitlab::Markdown::ExternalIssueReferenceFilter, @@ -109,8 +115,6 @@ module Gitlab Gitlab::Markdown::CommitRangeReferenceFilter, Gitlab::Markdown::CommitReferenceFilter, Gitlab::Markdown::LabelReferenceFilter, - - HTML::Pipeline::Gitlab::GitlabEmojiFilter ] end diff --git a/spec/helpers/gitlab_markdown_helper_spec.rb b/spec/helpers/gitlab_markdown_helper_spec.rb index 315e91d4f3..60e000b4dd 100644 --- a/spec/helpers/gitlab_markdown_helper_spec.rb +++ b/spec/helpers/gitlab_markdown_helper_spec.rb @@ -10,15 +10,12 @@ describe GitlabMarkdownHelper do end let!(:project) { create(:project) } - let(:empty_project) { create(:empty_project) } let(:user) { create(:user, username: 'gfm') } let(:commit) { project.repository.commit } - let(:earlier_commit){ project.repository.commit("HEAD~2") } let(:issue) { create(:issue, project: project) } let(:merge_request) { create(:merge_request, source_project: project, target_project: project) } let(:snippet) { create(:project_snippet, project: project) } - let(:member) { project.project_members.where(user_id: user).first } # Helper expects a current_user method. let(:current_user) { user } @@ -60,53 +57,6 @@ describe GitlabMarkdownHelper do end end - # TODO (rspeicher): These tests belong in the emoji filter spec - describe "emoji" do - it "matches at the start of a string" do - expect(gfm(":+1:")).to match(/ big time/) - end - - it "ignores invalid emoji" do - expect(gfm(":invalid-emoji:")).not_to match(/ Date: Mon, 20 Apr 2015 15:43:27 -0400 Subject: [PATCH 4/5] Use Nokogiri in `link_to_gfm` specs Didn't like relying so much on Regex matches --- spec/helpers/gitlab_markdown_helper_spec.rb | 42 +++++++++++---------- 1 file changed, 22 insertions(+), 20 deletions(-) diff --git a/spec/helpers/gitlab_markdown_helper_spec.rb b/spec/helpers/gitlab_markdown_helper_spec.rb index 60e000b4dd..11840a49e1 100644 --- a/spec/helpers/gitlab_markdown_helper_spec.rb +++ b/spec/helpers/gitlab_markdown_helper_spec.rb @@ -168,43 +168,45 @@ describe GitlabMarkdownHelper do end end - describe "#link_to_gfm" do + describe '#link_to_gfm' do let(:commit_path) { namespace_project_commit_path(project.namespace, project, commit) } let(:issues) { create_list(:issue, 2, project: project) } - it "should handle references nested in links with all the text" do + it 'should handle references nested in links with all the text' do actual = link_to_gfm("This should finally fix ##{issues[0].iid} and ##{issues[1].iid} for real", commit_path) - - # Break the result into groups of links with their content, without - # closing tags - groups = actual.split("") + doc = Nokogiri::HTML.parse(actual) # Leading commit link - expect(groups[0]).to match(/href="#{commit_path}"/) - expect(groups[0]).to match(/This should finally fix $/) + expect(doc.css('a')[0].attr('href')).to eq commit_path + expect(doc.css('a')[0].text).to eq 'This should finally fix ' # First issue link - expect(groups[1]). - to match(/href="#{namespace_project_issue_path(project.namespace, project, issues[0])}"/) - expect(groups[1]).to match(/##{issues[0].iid}$/) + expect(doc.css('a')[1].attr('href')). + to eq namespace_project_issue_path(project.namespace, project, issues[0]) + expect(doc.css('a')[1].text).to eq "##{issues[0].iid}" # Internal commit link - expect(groups[2]).to match(/href="#{commit_path}"/) - expect(groups[2]).to match(/ and /) + expect(doc.css('a')[2].attr('href')).to eq commit_path + expect(doc.css('a')[2].text).to eq ' and ' # Second issue link - expect(groups[3]). - to match(/href="#{namespace_project_issue_path(project.namespace, project, issues[1])}"/) - expect(groups[3]).to match(/##{issues[1].iid}$/) + expect(doc.css('a')[3].attr('href')). + to eq namespace_project_issue_path(project.namespace, project, issues[1]) + expect(doc.css('a')[3].text).to eq "##{issues[1].iid}" # Trailing commit link - expect(groups[4]).to match(/href="#{commit_path}"/) - expect(groups[4]).to match(/ for real$/) + expect(doc.css('a')[4].attr('href')).to eq commit_path + expect(doc.css('a')[4].text).to eq ' for real' end - it "should forward HTML options" do + it 'should forward HTML options' do actual = link_to_gfm("Fixed in #{commit.id}", commit_path, class: 'foo') - expect(actual).to have_selector 'a.gfm.gfm-commit.foo' + doc = Nokogiri::HTML.parse(actual) + + expect(doc.css('a')).to satisfy do |v| + # 'foo' gets added to all links + v.all? { |a| a.attr('class').match(/foo$/) } + end end it "escapes HTML passed in as the body" do From 870969a1b28e129f5ba73539528e81273b7e7b40 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Tue, 21 Apr 2015 13:12:19 -0400 Subject: [PATCH 5/5] More cleanup of gitlab_markdown_helper_spec --- spec/helpers/gitlab_markdown_helper_spec.rb | 176 ++++++++++---------- 1 file changed, 85 insertions(+), 91 deletions(-) diff --git a/spec/helpers/gitlab_markdown_helper_spec.rb b/spec/helpers/gitlab_markdown_helper_spec.rb index 11840a49e1..64f130e4ae 100644 --- a/spec/helpers/gitlab_markdown_helper_spec.rb +++ b/spec/helpers/gitlab_markdown_helper_spec.rb @@ -2,12 +2,6 @@ require 'spec_helper' describe GitlabMarkdownHelper do include ApplicationHelper - include IssuesHelper - - # TODO: Properly test this - def can?(*) - true - end let!(:project) { create(:project) } @@ -20,22 +14,15 @@ describe GitlabMarkdownHelper do # Helper expects a current_user method. let(:current_user) { user } - def url_helper(image_name) - File.join(root_url, 'assets', image_name) - end - before do # Helper expects a @project instance variable @project = project - @ref = 'markdown' - @repository = project.repository - @request.host = Gitlab.config.gitlab.host end describe "#gfm" do it "should forward HTML options to links" do expect(gfm("Fixed in #{commit.id}", @project, class: 'foo')). - to have_selector('a.gfm.foo') + to have_selector('a.gfm.foo') end describe "referencing multiple objects" do @@ -176,6 +163,9 @@ describe GitlabMarkdownHelper do actual = link_to_gfm("This should finally fix ##{issues[0].iid} and ##{issues[1].iid} for real", commit_path) doc = Nokogiri::HTML.parse(actual) + # Make sure we didn't create invalid markup + expect(doc.errors).to be_empty + # Leading commit link expect(doc.css('a')[0].attr('href')).to eq commit_path expect(doc.css('a')[0].text).to eq 'This should finally fix ' @@ -219,17 +209,6 @@ describe GitlabMarkdownHelper do describe "#markdown" do # TODO (rspeicher) - This block tests multiple different contexts. Break this up! - # REFERENCES (PART TWO: THE REVENGE) --------------------------------------- - - it "should handle references in headers" do - actual = "\n# Working around ##{issue.iid}\n## Apply !#{merge_request.iid}" - - expect(markdown(actual, no_header_anchors: true)). - to match(%r{Working around ##{issue.iid}}) - expect(markdown(actual, no_header_anchors: true)). - to match(%r{Apply !#{merge_request.iid}}) - end - it "should add ids and links to headers" do # Test every rule except nested tags. text = '..Ab_c-d. e..' @@ -245,6 +224,17 @@ describe GitlabMarkdownHelper do ) end + # REFERENCES (PART TWO: THE REVENGE) --------------------------------------- + + it "should handle references in headers" do + actual = "\n# Working around ##{issue.iid}\n## Apply !#{merge_request.iid}" + + expect(markdown(actual, no_header_anchors: true)). + to match(%r{Working around ##{issue.iid}}) + expect(markdown(actual, no_header_anchors: true)). + to match(%r{Apply !#{merge_request.iid}}) + end + it "should handle references in " do actual = "Apply _!#{merge_request.iid}_ ASAP" @@ -260,16 +250,15 @@ describe GitlabMarkdownHelper do target_html = "
some code from $#{snippet.id}\nhere too\n
\n" - expect(helper.markdown("\n some code from $#{snippet.id}\n here too\n")). + expect(markdown("\n some code from $#{snippet.id}\n here too\n")). to eq(target_html) - expect(helper.markdown("\n```\nsome code from $#{snippet.id}\nhere too\n```\n")). + expect(markdown("\n```\nsome code from $#{snippet.id}\nhere too\n```\n")). to eq(target_html) end it "should leave inline code untouched" do - expect(markdown("\nDon't use `$#{snippet.id}` here.\n")).to eq( - "

Don't use $#{snippet.id} here.

\n" - ) + expect(markdown("Don't use `$#{snippet.id}` here.")). + to eq "

Don't use $#{snippet.id} here.

\n" end # REF-LIKE AUTOLINKS? ----------------------------------------------------- @@ -287,67 +276,86 @@ describe GitlabMarkdownHelper do expect(markdown("screen shot: ![some image](http://example.tld/#!#{merge_request.iid})")).to eq("

screen shot: \"some

\n") end - it "should generate absolute urls for refs" do - expect(markdown("##{issue.iid}")).to include(namespace_project_issue_path(project.namespace, project, issue)) - end - # RELATIVE URLS ----------------------------------------------------------- # TODO (rspeicher): These belong in a relative link filter spec - it "should handle relative urls for a file in master" do - actual = "[GitLab API doc](doc/api/README.md)\n" - expected = "

GitLab API doc

\n" - expect(markdown(actual)).to match(expected) - end + context 'relative links' do + context 'with a valid repository' do + before do + @repository = project.repository + @ref = 'markdown' + end - it "should handle relative urls for a file in master with an anchor" do - actual = "[GitLab API doc](doc/api/README.md#section)\n" - expected = "

GitLab API doc

\n" - expect(markdown(actual)).to match(expected) - end + it "should handle relative urls for a file in master" do + actual = "[GitLab API doc](doc/api/README.md)\n" + expected = "

GitLab API doc

\n" + expect(markdown(actual)).to match(expected) + end - it "should not handle relative urls for the current file with an anchor" do - actual = "[GitLab API doc](#section)\n" - expected = "

GitLab API doc

\n" - expect(markdown(actual)).to match(expected) - end + it "should handle relative urls for a file in master with an anchor" do + actual = "[GitLab API doc](doc/api/README.md#section)\n" + expected = "

GitLab API doc

\n" + expect(markdown(actual)).to match(expected) + end - it "should handle relative urls for a directory in master" do - actual = "[GitLab API doc](doc/api)\n" - expected = "

GitLab API doc

\n" - expect(markdown(actual)).to match(expected) - end + it "should not handle relative urls for the current file with an anchor" do + actual = "[GitLab API doc](#section)\n" + expected = "

GitLab API doc

\n" + expect(markdown(actual)).to match(expected) + end - it "should handle absolute urls" do - actual = "[GitLab](https://www.gitlab.com)\n" - expected = "

GitLab

\n" - expect(markdown(actual)).to match(expected) - end + it "should handle relative urls for a directory in master" do + actual = "[GitLab API doc](doc/api)\n" + expected = "

GitLab API doc

\n" + expect(markdown(actual)).to match(expected) + end - it "should handle relative urls in reference links for a file in master" do - actual = "[GitLab API doc][GitLab readme]\n [GitLab readme]: doc/api/README.md\n" - expected = "

GitLab API doc

\n" - expect(markdown(actual)).to match(expected) - end + it "should handle absolute urls" do + actual = "[GitLab](https://www.gitlab.com)\n" + expected = "

GitLab

\n" + expect(markdown(actual)).to match(expected) + end - it "should handle relative urls in reference links for a directory in master" do - actual = "[GitLab API doc directory][GitLab readmes]\n [GitLab readmes]: doc/api/\n" - expected = "

GitLab API doc directory

\n" - expect(markdown(actual)).to match(expected) - end + it "should handle relative urls in reference links for a file in master" do + actual = "[GitLab API doc][GitLab readme]\n [GitLab readme]: doc/api/README.md\n" + expected = "

GitLab API doc

\n" + expect(markdown(actual)).to match(expected) + end - it "should not handle malformed relative urls in reference links for a file in master" do - actual = "[GitLab readme]: doc/api/README.md\n" - expected = "" - expect(markdown(actual)).to match(expected) - end + it "should handle relative urls in reference links for a directory in master" do + actual = "[GitLab API doc directory][GitLab readmes]\n [GitLab readmes]: doc/api/\n" + expected = "

GitLab API doc directory

\n" + expect(markdown(actual)).to match(expected) + end - it 'should allow whitelisted HTML tags from the user' do - actual = '
Term
Definition
' - expect(markdown(actual)).to match(actual) + it "should not handle malformed relative urls in reference links for a file in master" do + actual = "[GitLab readme]: doc/api/README.md\n" + expected = "" + expect(markdown(actual)).to match(expected) + end + + it 'should allow whitelisted HTML tags from the user' do + actual = '
Term
Definition
' + expect(markdown(actual)).to match(actual) + end + end + + context 'with an empty repository' do + before do + @project = create(:empty_project) + @repository = @project.repository + end + + it "should not touch relative urls" do + actual = "[GitLab API doc][GitLab readme]\n [GitLab readme]: doc/api/README.md\n" + expected = "

GitLab API doc

\n" + expect(markdown(actual)).to match(expected) + end + end end # SANITIZATION ------------------------------------------------------------ + # TODO (rspeicher): These are testing SanitizationFilter, not `markdown` it 'should sanitize tags that are not whitelisted' do actual = ' no blinks' @@ -375,20 +383,6 @@ describe GitlabMarkdownHelper do end end - # TODO (rspeicher): This should be a context of relative link specs, not its own thing - describe 'markdown for empty repository' do - before do - @project = create(:empty_project) - @repository = @project.repository - end - - it "should not touch relative urls" do - actual = "[GitLab API doc][GitLab readme]\n [GitLab readme]: doc/api/README.md\n" - expected = "

GitLab API doc

\n" - expect(markdown(actual)).to match(expected) - end - end - describe '#render_wiki_content' do before do @wiki = double('WikiPage')