From 1a09d5cda8e9f6b90b85351a16fcddea351b869f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jarka=20Kadlecov=C3=A1?= Date: Fri, 16 Feb 2018 14:33:50 +0100 Subject: [PATCH 1/2] Render htmlentities correctly for links not supported by Rinku --- changelogs/unreleased/41719-mr-title-fix.yml | 5 ++ lib/banzai/filter/autolink_filter.rb | 36 ++-------- lib/gitlab/string_range_marker.rb | 2 +- lib/gitlab/string_regex_marker.rb | 12 ++-- .../lib/banzai/filter/autolink_filter_spec.rb | 67 +++++++++++++------ spec/lib/gitlab/string_regex_marker_spec.rb | 35 +++++++--- 6 files changed, 90 insertions(+), 67 deletions(-) create mode 100644 changelogs/unreleased/41719-mr-title-fix.yml diff --git a/changelogs/unreleased/41719-mr-title-fix.yml b/changelogs/unreleased/41719-mr-title-fix.yml new file mode 100644 index 00000000000..92388f30cb2 --- /dev/null +++ b/changelogs/unreleased/41719-mr-title-fix.yml @@ -0,0 +1,5 @@ +--- +title: Render htmlentities correctly for links not supported by Rinku +merge_request: +author: +type: fixed diff --git a/lib/banzai/filter/autolink_filter.rb b/lib/banzai/filter/autolink_filter.rb index b8d2673c1a6..c4990637971 100644 --- a/lib/banzai/filter/autolink_filter.rb +++ b/lib/banzai/filter/autolink_filter.rb @@ -26,7 +26,7 @@ module Banzai # in the generated link. # # Rubular: http://rubular.com/r/cxjPyZc7Sb - LINK_PATTERN = %r{([a-z][a-z0-9\+\.-]+://\S+)(?]+)(?See #{link}" - expect(filter(act).to_html).to eq exp - end - end - - context 'when the input contains link' do - it 'does parse_html back the rinku returned value' do - act = HTML::Pipeline.parse("

See #{link}

") - - expect_any_instance_of(described_class).to receive(:parse_html).at_least(:once).and_call_original - - filter(act).to_html - end - end - end - - context 'other schemes' do - let(:link) { 'foo://bar.baz/' } - it 'autolinks smb' do link = 'smb:///Volumes/shared/foo.pdf' doc = filter("See #{link}") @@ -91,6 +85,21 @@ describe Banzai::Filter::AutolinkFilter do expect(doc.at_css('a')['href']).to eq link end + it 'autolinks multiple occurences of smb' do + link1 = 'smb:///Volumes/shared/foo.pdf' + link2 = 'smb:///Volumes/shared/bar.pdf' + + doc = filter("See #{link1} and #{link2}") + + found_links = doc.css('a') + + expect(found_links.size).to eq(2) + expect(found_links[0].text).to eq(link1) + expect(found_links[0]['href']).to eq(link1) + expect(found_links[1].text).to eq(link2) + expect(found_links[1]['href']).to eq(link2) + end + it 'autolinks irc' do link = 'irc://irc.freenode.net/git' doc = filter("See #{link}") @@ -151,4 +160,18 @@ describe Banzai::Filter::AutolinkFilter do end end end + + context 'when the link is inside a tag' do + it 'renders text after the link correctly for http' do + doc = filter(ERB::Util.html_escape_once("")) + + expect(doc.children.last.text).to include('') + end + + it 'renders text after the link correctly for not other protocol' do + doc = filter(ERB::Util.html_escape_once("")) + + expect(doc.children.last.text).to include('') + end + end end diff --git a/spec/lib/gitlab/string_regex_marker_spec.rb b/spec/lib/gitlab/string_regex_marker_spec.rb index d715f9bd641..37b1298b962 100644 --- a/spec/lib/gitlab/string_regex_marker_spec.rb +++ b/spec/lib/gitlab/string_regex_marker_spec.rb @@ -2,17 +2,36 @@ require 'spec_helper' describe Gitlab::StringRegexMarker do describe '#mark' do - let(:raw) { %{"name": "AFNetworking"} } - let(:rich) { %{"name": "AFNetworking"}.html_safe } - subject do - described_class.new(raw, rich).mark(/"[^"]+":\s*"(?[^"]+)"/, group: :name) do |text, left:, right:| - %{#{text}} + context 'with a single occurrence' do + let(:raw) { %{"name": "AFNetworking"} } + let(:rich) { %{"name": "AFNetworking"}.html_safe } + + subject do + described_class.new(raw, rich).mark(/"[^"]+":\s*"(?[^"]+)"/, group: :name) do |text, left:, right:| + %{#{text}} + end + end + + it 'marks the match' do + expect(subject).to eq(%{"name": "AFNetworking"}) + expect(subject).to be_html_safe end end - it 'marks the inline diffs' do - expect(subject).to eq(%{"name": "AFNetworking"}) - expect(subject).to be_html_safe + context 'with multiple occurrences' do + let(:raw) { %{a d} } + let(:rich) { %{a <b> <c> d}.html_safe } + + subject do + described_class.new(raw, rich).mark(/<[a-z]>/) do |text, left:, right:| + %{#{text}} + end + end + + it 'marks the matches' do + expect(subject).to eq(%{a <b> <c> d}) + expect(subject).to be_html_safe + end end end end From cb55bc3c0770adf7122d7cf49b12cb45c43de7ec Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Thu, 22 Feb 2018 12:09:27 +0000 Subject: [PATCH 2/2] Match Rinku's behaviour for closing punctuation in links Rinku 2.0.0 (the version we use) will remove the last character of a link if it's a closing part of a punctuation pair (different types of parentheses and quotes), unless both of the below are true: 1. The matching pair has different start and end characters. 2. There are equal numbers of both in the matched string (they don't have to be balanced). --- lib/banzai/filter/autolink_filter.rb | 50 ++++++++---- .../lib/banzai/filter/autolink_filter_spec.rb | 79 ++++++++++++++----- 2 files changed, 95 insertions(+), 34 deletions(-) diff --git a/lib/banzai/filter/autolink_filter.rb b/lib/banzai/filter/autolink_filter.rb index c4990637971..75b64ae9af2 100644 --- a/lib/banzai/filter/autolink_filter.rb +++ b/lib/banzai/filter/autolink_filter.rb @@ -25,7 +25,7 @@ module Banzai # period or comma for punctuation without those characters being included # in the generated link. # - # Rubular: http://rubular.com/r/cxjPyZc7Sb + # Rubular: http://rubular.com/r/JzPhi6DCZp LINK_PATTERN = %r{([a-z][a-z0-9\+\.-]+://[^\s>]+)(? "'", + '"' => '"', + ')' => '(', + ']' => '[', + '}' => '{' + }.freeze + def call return doc if context[:autolink] == false - text_parse - end - - private - - # Return true if any of the UNSAFE_PROTOCOLS strings are included in the URI scheme - def contains_unsafe?(scheme) - return false unless scheme - - scheme = scheme.strip.downcase - Banzai::Filter::SanitizationFilter::UNSAFE_PROTOCOLS.any? { |protocol| scheme.include?(protocol) } - end - - def text_parse doc.xpath(TEXT_QUERY).each do |node| content = node.to_html @@ -69,6 +63,16 @@ module Banzai doc end + private + + # Return true if any of the UNSAFE_PROTOCOLS strings are included in the URI scheme + def contains_unsafe?(scheme) + return false unless scheme + + scheme = scheme.strip.downcase + Banzai::Filter::SanitizationFilter::UNSAFE_PROTOCOLS.any? { |protocol| scheme.include?(protocol) } + end + def autolink_match(match) # start by stripping out dangerous links begin @@ -84,6 +88,22 @@ module Banzai match.gsub!(/((?:&[\w#]+;)+)\z/, '') dropped = ($1 || '').html_safe + # To match the behaviour of Rinku, if the matched link ends with a + # closing part of a matched pair of punctuation, we remove that trailing + # character unless there are an equal number of closing and opening + # characters in the link. + if match.end_with?(*PUNCTUATION_PAIRS.keys) + close_character = match[-1] + close_count = match.count(close_character) + open_character = PUNCTUATION_PAIRS[close_character] + open_count = match.count(open_character) + + if open_count != close_count || open_character == close_character + dropped += close_character + match = match[0..-2] + end + end + options = link_options.merge(href: match) content_tag(:a, match.html_safe, options) + dropped end diff --git a/spec/lib/banzai/filter/autolink_filter_spec.rb b/spec/lib/banzai/filter/autolink_filter_spec.rb index 0498b99ccf3..b502daea418 100644 --- a/spec/lib/banzai/filter/autolink_filter_spec.rb +++ b/spec/lib/banzai/filter/autolink_filter_spec.rb @@ -4,6 +4,7 @@ describe Banzai::Filter::AutolinkFilter do include FilterSpecHelper let(:link) { 'http://about.gitlab.com/' } + let(:quotes) { ['"', "'"] } it 'does nothing when :autolink is false' do exp = act = link @@ -15,16 +16,6 @@ describe Banzai::Filter::AutolinkFilter do expect(filter(act).to_html).to eq exp end - context 'when the input contains no links' do - it 'does not parse_html back the rinku returned value' do - act = HTML::Pipeline.parse('

This text contains no links to autolink

') - - expect_any_instance_of(described_class).not_to receive(:parse_html) - - filter(act).to_html - end - end - context 'Various schemes' do it 'autolinks http' do doc = filter("See #{link}") @@ -141,6 +132,45 @@ describe Banzai::Filter::AutolinkFilter do expect(doc.at_css('a').text).to eq link end + it 'includes trailing punctuation when part of a balanced pair' do + described_class::PUNCTUATION_PAIRS.each do |close, open| + next if open.in?(quotes) + + balanced_link = "#{link}#{open}abc#{close}" + balanced_actual = filter("See #{balanced_link}...") + unbalanced_link = "#{link}#{close}" + unbalanced_actual = filter("See #{unbalanced_link}...") + + expect(balanced_actual.at_css('a').text).to eq(balanced_link) + expect(unescape(balanced_actual.to_html)).to eq(Rinku.auto_link("See #{balanced_link}...")) + expect(unbalanced_actual.at_css('a').text).to eq(link) + expect(unescape(unbalanced_actual.to_html)).to eq(Rinku.auto_link("See #{unbalanced_link}...")) + end + end + + it 'removes trailing quotes' do + quotes.each do |quote| + balanced_link = "#{link}#{quote}abc#{quote}" + balanced_actual = filter("See #{balanced_link}...") + unbalanced_link = "#{link}#{quote}" + unbalanced_actual = filter("See #{unbalanced_link}...") + + expect(balanced_actual.at_css('a').text).to eq(balanced_link[0...-1]) + expect(unescape(balanced_actual.to_html)).to eq(Rinku.auto_link("See #{balanced_link}...")) + expect(unbalanced_actual.at_css('a').text).to eq(link) + expect(unescape(unbalanced_actual.to_html)).to eq(Rinku.auto_link("See #{unbalanced_link}...")) + end + end + + it 'removes one closing punctuation mark when the punctuation in the link is unbalanced' do + complicated_link = "(#{link}(a'b[c'd]))'" + expected_complicated_link = %Q{(#{link}(a'b[c'd]))'} + actual = unescape(filter(complicated_link).to_html) + + expect(actual).to eq(Rinku.auto_link(complicated_link)) + expect(actual).to eq(expected_complicated_link) + end + it 'does not include trailing HTML entities' do doc = filter("See <<<#{link}>>>") @@ -162,16 +192,27 @@ describe Banzai::Filter::AutolinkFilter do end context 'when the link is inside a tag' do - it 'renders text after the link correctly for http' do - doc = filter(ERB::Util.html_escape_once("")) + %w[http rdar].each do |protocol| + it "renders text after the link correctly for #{protocol}" do + doc = filter(ERB::Util.html_escape_once("<#{protocol}://link>")) - expect(doc.children.last.text).to include('') - end - - it 'renders text after the link correctly for not other protocol' do - doc = filter(ERB::Util.html_escape_once("")) - - expect(doc.children.last.text).to include('') + expect(doc.children.last.text).to include('') + end end end + + # Rinku does not escape these characters in HTML attributes, but content_tag + # does. We don't care about that difference for these specs, though. + def unescape(html) + %w([ ] { }).each do |cgi_escape| + html.sub!(CGI.escape(cgi_escape), cgi_escape) + end + + quotes.each do |html_escape| + html.sub!(CGI.escape_html(html_escape), html_escape) + html.sub!(CGI.escape(html_escape), CGI.escape_html(html_escape)) + end + + html + end end