diff --git a/ee/lib/ee/banzai/filter/references/epic_reference_filter.rb b/ee/lib/ee/banzai/filter/references/epic_reference_filter.rb index e8edd2586831e5b9f8d6b330c905455e9cb2c13e..80ad20809a91b6597414f93ff246b01012f07b6d 100644 --- a/ee/lib/ee/banzai/filter/references/epic_reference_filter.rb +++ b/ee/lib/ee/banzai/filter/references/epic_reference_filter.rb @@ -34,7 +34,7 @@ def reference_class(object_sym, tooltip: false) def data_attributes_for(text, group, object, link_content: false, link_reference: false) { - original: escape_html_entities(text), + original: text, link: link_content, link_reference: link_reference, group: group.id, diff --git a/ee/lib/ee/banzai/filter/references/label_reference_filter.rb b/ee/lib/ee/banzai/filter/references/label_reference_filter.rb index 6dedb186f558573065bcc845c4b4db430604e012..eee57295f27cb1cc1498cd4a007e9722ea1777f2 100644 --- a/ee/lib/ee/banzai/filter/references/label_reference_filter.rb +++ b/ee/lib/ee/banzai/filter/references/label_reference_filter.rb @@ -11,17 +11,8 @@ module LabelReferenceFilter def data_attributes_for(text, parent, object, link_content: false, link_reference: false) return super unless object.scoped_label? - # Enabling HTML tooltips for scoped labels here and additional escaping is done in `object_link_title` - super.merge!( - html: true - ) - end - - override :object_link_title - def object_link_title(object, matches) - return super unless object.scoped_label? - - ERB::Util.html_escape(super) + # Enabling HTML tooltips for scoped labels here. + super.merge!(html: true) end end end diff --git a/ee/lib/ee/banzai/filter/references/vulnerability_reference_filter.rb b/ee/lib/ee/banzai/filter/references/vulnerability_reference_filter.rb index d8bdb3080e1da01cd40efd4e8e46a30fdb07dc50..85611f630856ca93ec39e3e20a6f67b2b58afab4 100644 --- a/ee/lib/ee/banzai/filter/references/vulnerability_reference_filter.rb +++ b/ee/lib/ee/banzai/filter/references/vulnerability_reference_filter.rb @@ -35,7 +35,7 @@ def url_for_object(vulnerability, project) def data_attributes_for(text, project, object, link_content: false, link_reference: false) { - original: escape_html_entities(text), + original: text, link: link_content, link_reference: link_reference, project: project.id, diff --git a/ee/spec/lib/banzai/filter/references/label_reference_filter_spec.rb b/ee/spec/lib/banzai/filter/references/label_reference_filter_spec.rb index be45da7c74753f775316fdfb81b443c447f8a401..fc1aa60fd3904cb4ebdee3b446d5366b2b1a35a9 100644 --- a/ee/spec/lib/banzai/filter/references/label_reference_filter_spec.rb +++ b/ee/spec/lib/banzai/filter/references/label_reference_filter_spec.rb @@ -26,7 +26,7 @@ expect(doc.at_css('.gl-label-scoped a').attr('data-html')).to eq('true') end - it "escapes HTML in the label's title" do + it "doesn't unescape HTML in the label's title" do expect(doc.at_css('.gl-label-scoped a').attr('title')).to include('xss <svg id="svgId">') end end diff --git a/lib/banzai/filter/concerns/html_writer.rb b/lib/banzai/filter/concerns/html_writer.rb new file mode 100644 index 0000000000000000000000000000000000000000..cbb085ea003ac7578cd61c1821ba75b1e50f9a3d --- /dev/null +++ b/lib/banzai/filter/concerns/html_writer.rb @@ -0,0 +1,48 @@ +# frozen_string_literal: true + +module Banzai + module Filter + module Concerns + module HtmlWriter + extend ActiveSupport::Concern + + # Write an opening tag with the given tag name and attributes. + # + # The tag name and attribute names must not be user-specified; they are not validated. + # + # The attribute values are always escaped in the appropriate manner; the contract is that whatever value goes in + # is the logical DOM value of the attribute that comes out. + # + # i.e.: + # + # ```ruby + # html = write_opening_tag(tag, attrs) + # doc = Nokogiri::HTML.fragment(html) + # element = doc.child + # expect(element.to_h).to match(attrs) + # ``` + # + # This function should *never be changed*. + def write_opening_tag(tag_name, attributes) + s = +"<" << tag_name + attributes.each do |attr_name, attr_value| + next if attr_value.nil? + + s << " " << attr_name << "=" + s << '"' << CGI.escapeHTML(attr_value.to_s) << '"' + end + s << ">" + end + + # This is the most sure-fire correct way to implement the above function, relying on Nokogiri's DOM to correctly + # serialise attributes for us. Comparing the two with benchmark-ips and memory_profiler shows this method to be + # 7x slower and adds 3x memory pressure compared to the above. It is included here for reference. + # + # def write_opening_tag_nokogiri(tag_name, attributes) + # el = doc.document.create_element(tag_name, attributes.compact) + # el.to_html.chomp("") + # end + end + end + end +end diff --git a/lib/banzai/filter/references/abstract_reference_filter.rb b/lib/banzai/filter/references/abstract_reference_filter.rb index 205bab5582eb96decea94699e69c2f89634dab67..dc21b224b1d3a767297cbcc3eff2c6ba699374cb 100644 --- a/lib/banzai/filter/references/abstract_reference_filter.rb +++ b/lib/banzai/filter/references/abstract_reference_filter.rb @@ -230,9 +230,12 @@ def object_link_filter(text, pattern, link_content: nil, link_reference: false) content = context[:link_text] || link_content || object_link_text(object, matches) - link = %(#{content}) + link = write_opening_tag("a", { + "href" => url, + "title" => title, + "class" => klass, + **data + }) << content.to_s << "" wrap_link(link, object) else @@ -256,7 +259,7 @@ def data_attributes_for(text, parent, object, link_content: false, link_referenc end { - original: escape_html_entities(text), + original: text, link: link_content, link_reference: link_reference, object_sym => object.id diff --git a/lib/banzai/filter/references/external_issue_reference_filter.rb b/lib/banzai/filter/references/external_issue_reference_filter.rb index 7d7e7c86cbd4fcd7a1856060ed9e2eba34173a34..ff62f44efe1429a7accd182d2e531b912b8ca5f8 100644 --- a/lib/banzai/filter/references/external_issue_reference_filter.rb +++ b/lib/banzai/filter/references/external_issue_reference_filter.rb @@ -60,9 +60,12 @@ def object_link_filter(text, pattern, link_content: nil, link_reference: false) data = data_attribute(project: project.id, external_issue: id) content = link_content || match - %(#{content}) + write_opening_tag("a", { + "href" => url, + "title" => issue_title, + "class" => klass, + **data + }) << content.to_s << "" end end diff --git a/lib/banzai/filter/references/project_reference_filter.rb b/lib/banzai/filter/references/project_reference_filter.rb index 9b6c6fedb06c3dac1b9730e79d4bca3311d87929..52688a15245b75d3babd7496dbce8d39fc38680c 100644 --- a/lib/banzai/filter/references/project_reference_filter.rb +++ b/lib/banzai/filter/references/project_reference_filter.rb @@ -90,11 +90,12 @@ def link_to_project(project, link_content: nil) data = data_attribute(project: project.id) content = link_content || project.to_reference - link_tag(url, data, content, project.name) - end - - def link_tag(url, data, link_content, title) - %(#{link_content}) + write_opening_tag("a", { + "href" => url, + "title" => project.name, + "class" => link_class, + **data + }) << content << "" end end end diff --git a/lib/banzai/filter/references/reference_filter.rb b/lib/banzai/filter/references/reference_filter.rb index 12b3b6e591898f1ba77282a5c495e4c8cd98a6d1..07916f69e60f01a16d9b59fa598d7bcd10871a99 100644 --- a/lib/banzai/filter/references/reference_filter.rb +++ b/lib/banzai/filter/references/reference_filter.rb @@ -15,8 +15,10 @@ class ReferenceFilter < HTML::Pipeline::Filter include RequestStoreReferenceCache include Concerns::OutputSafety prepend Concerns::PipelineTimingCheck + include Concerns::HtmlWriter - REFERENCE_TYPE_DATA_ATTRIBUTE = 'data-reference-type=' + REFERENCE_TYPE_ATTRIBUTE = :reference_type + REFERENCE_TYPE_DATA_ATTRIBUTE_NAME = "data-#{REFERENCE_TYPE_ATTRIBUTE.to_s.dasherize}".freeze class << self # Implement in child class @@ -122,7 +124,7 @@ def requires_unescaping? private - # Returns a data attribute String to attach to a reference link + # Returns a Hash of attributes to attach to a reference link # # attributes - Hash, where the key becomes the data attribute name and the # value is the data attribute value @@ -130,28 +132,23 @@ def requires_unescaping? # Examples: # # data_attribute(project: 1, issue: 2) - # # => "data-reference-type=\"SomeReferenceFilter\" data-project=\"1\" data-issue=\"2\"" + # # => {"data-reference-type" => "SomeReferenceFilter", "data-container" => "body", + # "data-placement" => "top", "data-project" => 1, "data-issue" => 2} # # data_attribute(project: 3, merge_request: 4) - # # => "data-reference-type=\"SomeReferenceFilter\" data-project=\"3\" data-merge-request=\"4\"" + # # => {"data-reference-type" => "SomeReferenceFilter", "data-container" => "body", + # "data-placement" => "top", "data-project" => 3, "data-merge-request" => 4} # - # Returns a String + # Returns a Hash def data_attribute(attributes = {}) - attributes = attributes.reject { |_, v| v.nil? } - - # "data-reference-type=" attribute got moved into a constant because we need - # to use it on ReferenceRewriter class to detect if the markdown contains any reference - reference_type_attribute = "#{REFERENCE_TYPE_DATA_ATTRIBUTE}#{escape_once(self.class.reference_type)} " + attributes = attributes.compact attributes[:container] ||= 'body' attributes[:placement] ||= 'top' + attributes[REFERENCE_TYPE_ATTRIBUTE] = self.class.reference_type attributes.delete(:original) if context[:no_original_data] - attributes.map do |key, value| - %(data-#{key.to_s.dasherize}="#{escape_once(value)}") - end - .join(' ') - .prepend(reference_type_attribute) + attributes.transform_keys { |k| "data-#{k.to_s.dasherize}" } end def ignore_ancestor_query diff --git a/lib/banzai/filter/references/user_reference_filter.rb b/lib/banzai/filter/references/user_reference_filter.rb index cbfdcf037e568f2bbb68133e0ade2b496cbefbc2..f6ea1713bb9f36e573856502bc955f13d76b2efd 100644 --- a/lib/banzai/filter/references/user_reference_filter.rb +++ b/lib/banzai/filter/references/user_reference_filter.rb @@ -155,7 +155,12 @@ def link_to_org_user_detail(org_user_detail, link_content: nil) end def link_tag(url, data, link_content, title) - %(#{link_content}) + write_opening_tag("a", { + "href" => url, + "title" => title, + "class" => link_class, + **data + }) << link_content << "" end def organization diff --git a/lib/gitlab/gfm/reference_rewriter.rb b/lib/gitlab/gfm/reference_rewriter.rb index 606c63e5001cc02c2d3a829b936fe1b577522e24..d4fec5e89752de583623004ceda30e382341b0a8 100644 --- a/lib/gitlab/gfm/reference_rewriter.rb +++ b/lib/gitlab/gfm/reference_rewriter.rb @@ -56,9 +56,9 @@ def rewrite(target_parent) def needs_rewrite? strong_memoize(:needs_rewrite) do reference_type_attribute = - Banzai::Filter::References::ReferenceFilter::REFERENCE_TYPE_DATA_ATTRIBUTE + Banzai::Filter::References::ReferenceFilter::REFERENCE_TYPE_DATA_ATTRIBUTE_NAME - @text_html.include?(reference_type_attribute) + @text_html.include?("#{reference_type_attribute}=") end end diff --git a/spec/lib/banzai/filter/references/abstract_reference_filter_spec.rb b/spec/lib/banzai/filter/references/abstract_reference_filter_spec.rb index 74c7c89c629da99a6b97b9f91ed214504354e1f0..a2e0d92328d7294547fec9be182c3f85d4c20c9a 100644 --- a/spec/lib/banzai/filter/references/abstract_reference_filter_spec.rb +++ b/spec/lib/banzai/filter/references/abstract_reference_filter_spec.rb @@ -16,7 +16,7 @@ data_attributes = filter_instance.data_attributes_for('xss <img onerror=alert(1) src=x>', project, issue, link_content: true) - expect(data_attributes[:original]).to eq('xss &lt;img onerror=alert(1) src=x&gt;') + expect(data_attributes[:original]).to eq('xss <img onerror=alert(1) src=x>') end end diff --git a/spec/lib/banzai/filter/references/commit_range_reference_filter_spec.rb b/spec/lib/banzai/filter/references/commit_range_reference_filter_spec.rb index 5179c2fedd32731af7d33cf47f34c97d4a576aad..1e1fec8b542d7d3ff547fcff814aee1ccf6326d2 100644 --- a/spec/lib/banzai/filter/references/commit_range_reference_filter_spec.rb +++ b/spec/lib/banzai/filter/references/commit_range_reference_filter_spec.rb @@ -68,7 +68,7 @@ it 'includes no title attribute' do doc = reference_filter("See #{reference}") - expect(doc.css('a').first.attr('title')).to eq "" + expect(doc.css('a').first.attr('title')).to be_nil end it 'includes default classes' do diff --git a/spec/lib/banzai/filter/references/milestone_reference_filter_spec.rb b/spec/lib/banzai/filter/references/milestone_reference_filter_spec.rb index 8eaf629c24c651229fd7bed12ca27b6a16a59337..37269a0c675a3e0f9f4fb2a123466ce9fe0b6023 100644 --- a/spec/lib/banzai/filter/references/milestone_reference_filter_spec.rb +++ b/spec/lib/banzai/filter/references/milestone_reference_filter_spec.rb @@ -340,7 +340,7 @@ result = reference_filter("See #{absolute_reference}") expect(result.css('a').first.attr('href')).to eq(urls.milestone_url(milestone)) - expect(result.css('a').first.attr('data-original')).to eq absolute_reference + expect(result.css('a').first.attr('data-original')).to eq(absolute_reference) expect(result.content).to eq "See %#{milestone.title}" end end diff --git a/spec/lib/banzai/reference_redactor_spec.rb b/spec/lib/banzai/reference_redactor_spec.rb index e3c66d8075180faeac04d485885d7e412e2f6dab..b7d991553dcf4ced29cf1ef46bb5b8ef7b9e8ce4 100644 --- a/spec/lib/banzai/reference_redactor_spec.rb +++ b/spec/lib/banzai/reference_redactor_spec.rb @@ -68,7 +68,8 @@ end it 'redacts an issue attached' do - doc = Nokogiri::HTML.fragment("foo") + doc = Nokogiri::HTML.fragment("foo") + doc.css('a').first["data-issue"] = issue.id redactor.redact([doc]) @@ -76,7 +77,9 @@ end it 'redacts an external issue' do - doc = Nokogiri::HTML.fragment("foo") + doc = Nokogiri::HTML.fragment("foo") + doc.css('a').first["data-external-issue"] = issue.id + doc.css('a').first["data-project"] = project.id redactor.redact([doc]) diff --git a/spec/requests/projects/incident_management/timeline_events_spec.rb b/spec/requests/projects/incident_management/timeline_events_spec.rb index fee3ef6e322c572129f197d18e24c867cd1894da..8b866dcea5dda73f8e8844bc7ef502d049f9f301 100644 --- a/spec/requests/projects/incident_management/timeline_events_spec.rb +++ b/spec/requests/projects/incident_management/timeline_events_spec.rb @@ -16,12 +16,12 @@ end let(:expected_reference) do - "##{incident.iid}" + "#1" end let(:expected_body) do