Commit 6b4c6fa1 authored by Jan Zdráhal's avatar Jan Zdráhal Committed by Sean McGivern

Fix Markdown styling inside reference links

Fixes: https://gitlab.com/gitlab-org/gitlab-ce/issues/18096
parent 266fcfb1
......@@ -7,6 +7,7 @@ Please view this file on the master branch, on stable branches it's out of date.
- Trim leading and trailing whitespace on project_path (Linus Thiel)
- Prevent award emoji via notes for issues/MRs authored by user (barthc)
- Adds an optional path parameter to the Commits API to filter commits by path (Luis HGO)
- Fix Markdown styling inside reference links (Jan Zdráhal)
- Fix extra space on Build sidebar on Firefox !7060
- Fix mobile layout issues in admin user overview page !7087
- Fix HipChat notifications rendering (airatshigapov, eisnerd)
......
......@@ -102,10 +102,10 @@ module Banzai
end
elsif element_node?(node)
yield_valid_link(node) do |link, text|
yield_valid_link(node) do |link, inner_html|
if ref_pattern && link =~ /\A#{ref_pattern}\z/
replace_link_node_with_href(node, link) do
object_link_filter(link, ref_pattern, link_text: text)
object_link_filter(link, ref_pattern, link_content: inner_html)
end
next
......@@ -113,9 +113,9 @@ module Banzai
next unless link_pattern
if link == text && text =~ /\A#{link_pattern}/
if link == inner_html && inner_html =~ /\A#{link_pattern}/
replace_link_node_with_text(node, link) do
object_link_filter(text, link_pattern)
object_link_filter(inner_html, link_pattern)
end
next
......@@ -123,7 +123,7 @@ module Banzai
if link =~ /\A#{link_pattern}\z/
replace_link_node_with_href(node, link) do
object_link_filter(link, link_pattern, link_text: text)
object_link_filter(link, link_pattern, link_content: inner_html)
end
next
......@@ -140,11 +140,11 @@ module Banzai
#
# text - String text to replace references in.
# pattern - Reference pattern to match against.
# link_text - Original content of the link being replaced.
# link_content - Original content of the link being replaced.
#
# Returns a String with references replaced with links. All links
# have `gfm` and `gfm-OBJECT_NAME` class names attached for styling.
def object_link_filter(text, pattern, link_text: nil)
def object_link_filter(text, pattern, link_content: nil)
references_in(text, pattern) do |match, id, project_ref, matches|
project = project_from_ref_cached(project_ref)
......@@ -152,7 +152,7 @@ module Banzai
title = object_link_title(object)
klass = reference_class(object_sym)
data = data_attributes_for(link_text || match, project, object)
data = data_attributes_for(link_content || match, project, object)
if matches.names.include?("url") && matches[:url]
url = matches[:url]
......@@ -160,11 +160,11 @@ module Banzai
url = url_for_object_cached(object, project)
end
text = link_text || object_link_text(object, matches)
content = link_content || object_link_text(object, matches)
%(<a href="#{url}" #{data}
title="#{escape_once(title)}"
class="#{klass}">#{escape_once(text)}</a>)
class="#{klass}">#{content}</a>)
else
match
end
......
......@@ -37,10 +37,10 @@ module Banzai
end
elsif element_node?(node)
yield_valid_link(node) do |link, text|
yield_valid_link(node) do |link, inner_html|
if link =~ ref_start_pattern
replace_link_node_with_href(node, link) do
issue_link_filter(link, link_text: text)
issue_link_filter(link, link_content: inner_html)
end
end
end
......@@ -54,10 +54,11 @@ module Banzai
# issue's details page.
#
# text - String text to replace references in.
# link_content - Original content of the link being replaced.
#
# Returns a String with `JIRA-123` references replaced with links. All
# links have `gfm` and `gfm-issue` class names attached for styling.
def issue_link_filter(text, link_text: nil)
def issue_link_filter(text, link_content: nil)
project = context[:project]
self.class.references_in(text, issue_reference_pattern) do |match, id|
......@@ -69,11 +70,11 @@ module Banzai
klass = reference_class(:issue)
data = data_attribute(project: project.id, external_issue: id)
text = link_text || match
content = link_content || match
%(<a href="#{url}" #{data}
title="#{escape_once(title)}"
class="#{klass}">#{escape_once(text)}</a>)
class="#{klass}">#{content}</a>)
end
end
......
......@@ -85,14 +85,14 @@ module Banzai
@nodes ||= each_node.to_a
end
# Yields the link's URL and text whenever the node is a valid <a> tag.
# Yields the link's URL and inner HTML whenever the node is a valid <a> tag.
def yield_valid_link(node)
link = CGI.unescape(node.attr('href').to_s)
text = node.text
inner_html = node.inner_html
return unless link.force_encoding('UTF-8').valid_encoding?
yield link, text
yield link, inner_html
end
def replace_text_when_pattern_matches(node, pattern)
......
......@@ -35,10 +35,10 @@ module Banzai
user_link_filter(content)
end
elsif element_node?(node)
yield_valid_link(node) do |link, text|
yield_valid_link(node) do |link, inner_html|
if link =~ ref_pattern_start
replace_link_node_with_href(node, link) do
user_link_filter(link, link_text: text)
user_link_filter(link, link_content: inner_html)
end
end
end
......@@ -52,15 +52,16 @@ module Banzai
# user's profile page.
#
# text - String text to replace references in.
# link_content - Original content of the link being replaced.
#
# Returns a String with `@user` references replaced with links. All links
# have `gfm` and `gfm-project_member` class names attached for styling.
def user_link_filter(text, link_text: nil)
def user_link_filter(text, link_content: nil)
self.class.references_in(text) do |match, username|
if username == 'all'
link_to_all(link_text: link_text)
link_to_all(link_content: link_content)
elsif namespace = namespaces[username]
link_to_namespace(namespace, link_text: link_text) || match
link_to_namespace(namespace, link_content: link_content) || match
else
match
end
......@@ -102,49 +103,49 @@ module Banzai
reference_class(:project_member)
end
def link_to_all(link_text: nil)
def link_to_all(link_content: nil)
project = context[:project]
author = context[:author]
if author && !project.team.member?(author)
link_text
link_content
else
url = urls.namespace_project_url(project.namespace, project,
only_path: context[:only_path])
data = data_attribute(project: project.id, author: author.try(:id))
text = link_text || User.reference_prefix + 'all'
content = link_content || User.reference_prefix + 'all'
link_tag(url, data, text, 'All Project and Group Members')
link_tag(url, data, content, 'All Project and Group Members')
end
end
def link_to_namespace(namespace, link_text: nil)
def link_to_namespace(namespace, link_content: nil)
if namespace.is_a?(Group)
link_to_group(namespace.path, namespace, link_text: link_text)
link_to_group(namespace.path, namespace, link_content: link_content)
else
link_to_user(namespace.path, namespace, link_text: link_text)
link_to_user(namespace.path, namespace, link_content: link_content)
end
end
def link_to_group(group, namespace, link_text: nil)
def link_to_group(group, namespace, link_content: nil)
url = urls.group_url(group, only_path: context[:only_path])
data = data_attribute(group: namespace.id)
text = link_text || Group.reference_prefix + group
content = link_content || Group.reference_prefix + group
link_tag(url, data, text, namespace.name)
link_tag(url, data, content, namespace.name)
end
def link_to_user(user, namespace, link_text: nil)
def link_to_user(user, namespace, link_content: nil)
url = urls.user_url(user, only_path: context[:only_path])
data = data_attribute(user: namespace.owner_id)
text = link_text || User.reference_prefix + user
content = link_content || User.reference_prefix + user
link_tag(url, data, text, namespace.owner_name)
link_tag(url, data, content, namespace.owner_name)
end
def link_tag(url, data, text, title)
%(<a href="#{url}" #{data} class="#{link_class}" title="#{escape_once(title)}">#{escape_once(text)}</a>)
def link_tag(url, data, link_content, title)
%(<a href="#{url}" #{data} class="#{link_class}" title="#{escape_once(title)}">#{link_content}</a>)
end
end
end
......
......@@ -41,10 +41,10 @@ module Banzai
next if visible.include?(node)
doc_data[:visible_reference_count] -= 1
# The reference should be replaced by the original text,
# which is not always the same as the rendered text.
text = node.attr('data-original') || node.text
node.replace(text)
# The reference should be replaced by the original link's content,
# which is not always the same as the rendered one.
content = node.attr('data-original') || node.inner_html
node.replace(content)
end
end
......
......@@ -8,6 +8,8 @@ describe Banzai::Filter::ExternalIssueReferenceFilter, lib: true do
end
shared_examples_for "external issue tracker" do
it_behaves_like 'a reference containing an element node'
it 'requires project context' do
expect { described_class.call('') }.to raise_error(ArgumentError, /:project/)
end
......
......@@ -22,6 +22,8 @@ describe Banzai::Filter::IssueReferenceFilter, lib: true do
end
context 'internal reference' do
it_behaves_like 'a reference containing an element node'
let(:reference) { issue.to_reference }
it 'ignores valid references when using non-default tracker' do
......@@ -83,6 +85,20 @@ describe Banzai::Filter::IssueReferenceFilter, lib: true do
expect(link.attr('data-issue')).to eq issue.id.to_s
end
it 'includes a data-original attribute' do
doc = reference_filter("See #{reference}")
link = doc.css('a').first
expect(link).to have_attribute('data-original')
expect(link.attr('data-original')).to eq reference
end
it 'does not escape the data-original attribute' do
inner_html = 'element <code>node</code> inside'
doc = reference_filter(%{<a href="#{reference}">#{inner_html}</a>})
expect(doc.children.first.attr('data-original')).to eq inner_html
end
it 'supports an :only_path context' do
doc = reference_filter("Issue #{reference}", only_path: true)
link = doc.css('a').first.attr('href')
......@@ -101,6 +117,8 @@ describe Banzai::Filter::IssueReferenceFilter, lib: true do
end
context 'cross-project reference' do
it_behaves_like 'a reference containing an element node'
let(:namespace) { create(:namespace, name: 'cross-reference') }
let(:project2) { create(:empty_project, :public, namespace: namespace) }
let(:issue) { create(:issue, project: project2) }
......@@ -141,6 +159,8 @@ describe Banzai::Filter::IssueReferenceFilter, lib: true do
end
context 'cross-project URL reference' do
it_behaves_like 'a reference containing an element node'
let(:namespace) { create(:namespace, name: 'cross-reference') }
let(:project2) { create(:empty_project, :public, namespace: namespace) }
let(:issue) { create(:issue, project: project2) }
......@@ -160,39 +180,45 @@ describe Banzai::Filter::IssueReferenceFilter, lib: true do
end
context 'cross-project reference in link href' do
it_behaves_like 'a reference containing an element node'
let(:namespace) { create(:namespace, name: 'cross-reference') }
let(:project2) { create(:empty_project, :public, namespace: namespace) }
let(:issue) { create(:issue, project: project2) }
let(:reference) { %Q{<a href="#{issue.to_reference(project)}">Reference</a>} }
let(:reference) { issue.to_reference(project) }
let(:reference_link) { %{<a href="#{reference}">Reference</a>} }
it 'links to a valid reference' do
doc = reference_filter("See #{reference}")
doc = reference_filter("See #{reference_link}")
expect(doc.css('a').first.attr('href')).
to eq helper.url_for_issue(issue.iid, project2)
end
it 'links with adjacent text' do
doc = reference_filter("Fixed (#{reference}.)")
doc = reference_filter("Fixed (#{reference_link}.)")
expect(doc.to_html).to match(/\(<a.+>Reference<\/a>\.\)/)
end
end
context 'cross-project URL in link href' do
it_behaves_like 'a reference containing an element node'
let(:namespace) { create(:namespace, name: 'cross-reference') }
let(:project2) { create(:empty_project, :public, namespace: namespace) }
let(:issue) { create(:issue, project: project2) }
let(:reference) { %Q{<a href="#{helper.url_for_issue(issue.iid, project2) + "#note_123"}">Reference</a>} }
let(:reference) { "#{helper.url_for_issue(issue.iid, project2) + "#note_123"}" }
let(:reference_link) { %{<a href="#{reference}">Reference</a>} }
it 'links to a valid reference' do
doc = reference_filter("See #{reference}")
doc = reference_filter("See #{reference_link}")
expect(doc.css('a').first.attr('href')).
to eq helper.url_for_issue(issue.iid, project2) + "#note_123"
end
it 'links with adjacent text' do
doc = reference_filter("Fixed (#{reference}.)")
doc = reference_filter("Fixed (#{reference_link}.)")
expect(doc.to_html).to match(/\(<a.+>Reference<\/a>\.\)/)
end
end
......
......@@ -24,6 +24,8 @@ describe Banzai::Filter::UserReferenceFilter, lib: true do
end
context 'mentioning @all' do
it_behaves_like 'a reference containing an element node'
let(:reference) { User.reference_prefix + 'all' }
before do
......@@ -60,6 +62,8 @@ describe Banzai::Filter::UserReferenceFilter, lib: true do
end
context 'mentioning a user' do
it_behaves_like 'a reference containing an element node'
it 'links to a User' do
doc = reference_filter("Hey #{reference}")
expect(doc.css('a').first.attr('href')).to eq urls.user_url(user)
......@@ -89,6 +93,8 @@ describe Banzai::Filter::UserReferenceFilter, lib: true do
end
context 'mentioning a group' do
it_behaves_like 'a reference containing an element node'
let(:group) { create(:group) }
let(:reference) { group.to_reference }
......
require 'rails_helper'
describe Banzai::Pipeline::FullPipeline do
describe 'References' do
let(:project) { create(:empty_project, :public) }
let(:issue) { create(:issue, project: project) }
it 'handles markdown inside a reference' do
markdown = "[some `code` inside](#{issue.to_reference})"
result = described_class.call(markdown, project: project)
link_content = result[:output].css('a').inner_html
expect(link_content).to eq('some <code>code</code> inside')
end
it 'sanitizes reference HTML' do
link_label = '<script>bad things</script>'
markdown = "[#{link_label}](#{issue.to_reference})"
result = described_class.to_html(markdown, project: project)
expect(result).not_to include(link_label)
end
it 'escapes the data-original attribute on a reference' do
markdown = %Q{[">bad things](#{issue.to_reference})}
result = described_class.to_html(markdown, project: project)
expect(result).to include(%{data-original='\"&gt;bad things'})
end
end
end
......@@ -6,39 +6,60 @@ describe Banzai::Redactor do
let(:redactor) { described_class.new(project, user) }
describe '#redact' do
it 'redacts an Array of documents' do
doc1 = Nokogiri::HTML.
fragment('<a class="gfm" data-reference-type="issue">foo</a>')
doc2 = Nokogiri::HTML.
fragment('<a class="gfm" data-reference-type="issue">bar</a>')
expect(redactor).to receive(:nodes_visible_to_user).and_return([])
redacted_data = redactor.redact([doc1, doc2])
expect(redacted_data.map { |data| data[:document] }).to eq([doc1, doc2])
expect(redacted_data.map { |data| data[:visible_reference_count] }).to eq([0, 0])
expect(doc1.to_html).to eq('foo')
expect(doc2.to_html).to eq('bar')
context 'when reference not visible to user' do
before do
expect(redactor).to receive(:nodes_visible_to_user).and_return([])
end
it 'redacts an array of documents' do
doc1 = Nokogiri::HTML.
fragment('<a class="gfm" data-reference-type="issue">foo</a>')
doc2 = Nokogiri::HTML.
fragment('<a class="gfm" data-reference-type="issue">bar</a>')
redacted_data = redactor.redact([doc1, doc2])
expect(redacted_data.map { |data| data[:document] }).to eq([doc1, doc2])
expect(redacted_data.map { |data| data[:visible_reference_count] }).to eq([0, 0])
expect(doc1.to_html).to eq('foo')
expect(doc2.to_html).to eq('bar')
end
it 'replaces redacted reference with inner HTML' do
doc = Nokogiri::HTML.fragment("<a class='gfm' data-reference-type='issue'>foo</a>")
redactor.redact([doc])
expect(doc.to_html).to eq('foo')
end
context 'when data-original attribute provided' do
let(:original_content) { '<code>foo</code>' }
it 'replaces redacted reference with original content' do
doc = Nokogiri::HTML.fragment("<a class='gfm' data-reference-type='issue' data-original='#{original_content}'>bar</a>")
redactor.redact([doc])
expect(doc.to_html).to eq(original_content)
end
end
end
it 'does not redact an Array of documents' do
doc1_html = '<a class="gfm" data-reference-type="issue">foo</a>'
doc1 = Nokogiri::HTML.fragment(doc1_html)
context 'when reference visible to user' do
it 'does not redact an array of documents' do
doc1_html = '<a class="gfm" data-reference-type="issue">foo</a>'
doc1 = Nokogiri::HTML.fragment(doc1_html)
doc2_html = '<a class="gfm" data-reference-type="issue">bar</a>'
doc2 = Nokogiri::HTML.fragment(doc2_html)
doc2_html = '<a class="gfm" data-reference-type="issue">bar</a>'
doc2 = Nokogiri::HTML.fragment(doc2_html)
nodes = redactor.document_nodes([doc1, doc2]).map { |x| x[:nodes] }
expect(redactor).to receive(:nodes_visible_to_user).and_return(nodes.flatten)
nodes = redactor.document_nodes([doc1, doc2]).map { |x| x[:nodes] }
expect(redactor).to receive(:nodes_visible_to_user).and_return(nodes.flatten)
redacted_data = redactor.redact([doc1, doc2])
redacted_data = redactor.redact([doc1, doc2])
expect(redacted_data.map { |data| data[:document] }).to eq([doc1, doc2])
expect(redacted_data.map { |data| data[:visible_reference_count] }).to eq([1, 1])
expect(doc1.to_html).to eq(doc1_html)
expect(doc2.to_html).to eq(doc2_html)
expect(redacted_data.map { |data| data[:document] }).to eq([doc1, doc2])
expect(redacted_data.map { |data| data[:visible_reference_count] }).to eq([1, 1])
expect(doc1.to_html).to eq(doc1_html)
expect(doc2.to_html).to eq(doc2_html)
end
end
end
......
# Specs for reference links containing HTML.
#
# Requires a reference:
# let(:reference) { '#42' }
shared_examples 'a reference containing an element node' do
let(:inner_html) { 'element <code>node</code> inside' }
let(:reference_with_element) { %{<a href="#{reference}">#{inner_html}</a>} }
it 'does not escape inner html' do
doc = reference_filter(reference_with_element)
expect(doc.children.first.inner_html).to eq(inner_html)
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