diff --git a/app/helpers/labels_helper.rb b/app/helpers/labels_helper.rb index 6b7dc73be88b380343a7bfe545214edd551a9f71..0b21ee4fccc99e2efd0aded93bdb4650a591d7d3 100644 --- a/app/helpers/labels_helper.rb +++ b/app/helpers/labels_helper.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true module LabelsHelper + include ActionView::Helpers::TagHelper extend self def show_label_issuables_link?(label, issuables_type, current_user: nil) @@ -35,7 +36,7 @@ def show_label_issuables_link?(label, issuables_type, current_user: nil) # # Customize link body with a block # link_to_label(label) { "My Custom Label Text" } # - # Returns a String + # Returns a String containing HTML. def link_to_label(label, type: :issue, tooltip: true, css_class: nil, &block) link = label.filter_path(type: type) @@ -46,6 +47,7 @@ def link_to_label(label, type: :issue, tooltip: true, css_class: nil, &block) end end + # Returns a String containing HTML. def render_label(label, link: nil, tooltip: true, dataset: nil, tooltip_shows_title: false) html = render_colored_label(label) @@ -57,10 +59,11 @@ def render_label(label, link: nil, tooltip: true, dataset: nil, tooltip_shows_ti wrap_label_html(html, label: label) end - def render_colored_label(label, suffix: '') + # Renders a label, with the given suffix HTML. + def render_colored_label(label, in_reference: nil) render_label_text( label.name, - suffix: suffix, + in_reference: in_reference, css_class: "gl-label-text #{label.text_color_class}", bg_color: label.color ) @@ -73,8 +76,9 @@ def wrap_label_html(label_html, label:) %(#{label_html}).html_safe end + # Returns a String containing text. def label_tooltip_title(label, tooltip_shows_title: false) - Sanitize.clean(tooltip_shows_title ? label.title : label.description) + tooltip_shows_title ? label.title : label.description end def suggested_colors @@ -224,15 +228,13 @@ def render_label_link(label_html, link:, title:, dataset:) link_to(label_html, link, class: classes.join(' '), data: dataset) end - def render_label_text(name, suffix: '', css_class: nil, bg_color: nil) - <<~HTML.chomp.html_safe - #{ERB::Util.html_escape_once(name)}#{suffix} - HTML + def render_label_text(name, in_reference: nil, css_class: nil, bg_color: nil) + label_name_html = CGI.escapeHTML(name) + label_name_html << " " << content_tag(:i, "in #{in_reference}") if in_reference.present? + + attrs = { "class" => css_class, "data-container" => "body", "data-html" => "true" } + attrs["style"] = "background-color: #{bg_color}" if bg_color + content_tag(:span, label_name_html.html_safe, **attrs) end end diff --git a/ee/app/helpers/ee/labels_helper.rb b/ee/app/helpers/ee/labels_helper.rb index 26b926ccc4d5f54c3196a909f34446670518942f..bc898baec84e72efbe0662c44050ff9f57ce20b9 100644 --- a/ee/app/helpers/ee/labels_helper.rb +++ b/ee/app/helpers/ee/labels_helper.rb @@ -8,7 +8,7 @@ module LabelsHelper singleton_class.prepend self end - def render_colored_label(label, suffix: '') + def render_colored_label(label, in_reference: nil) return super unless label.scoped_label? render_label_text( @@ -18,7 +18,7 @@ def render_colored_label(label, suffix: '') ) + render_label_text( label.scoped_label_value, css_class: "gl-label-text-scoped", - suffix: suffix + in_reference: in_reference ) end @@ -31,11 +31,12 @@ def wrap_label_html(label_html, label:) %(#{label_html}).html_safe end - def label_tooltip_title(label, tooltip_shows_title: false) - tooltip = super - tooltip = %(Scoped label
#{tooltip}) if label.scoped_label? + # Returns a String containing HTML. + def label_tooltip_title_html(label) + tooltip_html = CGI.escapeHTML(label_tooltip_title(label).to_s) + tooltip_html = %(Scoped label
#{tooltip_html}) if label.scoped_label? - tooltip + tooltip_html end def label_dropdown_data(edit_context, opts = {}) 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 6cdab6d09621e090253d4ac4553d97ea0da5ee05..8bee34df542ad8275dc128fabbea23febf9fc3b4 100644 --- a/ee/lib/ee/banzai/filter/references/label_reference_filter.rb +++ b/ee/lib/ee/banzai/filter/references/label_reference_filter.rb @@ -9,11 +9,18 @@ module LabelReferenceFilter override :data_attributes_for def data_attributes_for(original, parent, object, link_content: false, link_reference: false) - return super unless object.scoped_label? - - # Enabling HTML tooltips for scoped labels here. + # Causes the title (returned by `object_link_title` below) to be interpreted + # as HTML. super.merge!(html: true) end + + # Returns a String containing HTML. + override :object_link_title + def object_link_title(object, _matches) + presenter = object.present(issuable_subject: project || group) + + ::LabelsHelper.label_tooltip_title_html(presenter) + end end end end diff --git a/ee/spec/helpers/ee/labels_helper_spec.rb b/ee/spec/helpers/ee/labels_helper_spec.rb index cdfb7927013b46df3ae1729257937d37882beb66..f9e6816a27bf0d6b005cdb0ed4d63152c3c3a476 100644 --- a/ee/spec/helpers/ee/labels_helper_spec.rb +++ b/ee/spec/helpers/ee/labels_helper_spec.rb @@ -123,4 +123,28 @@ } end end + + describe '#label_tooltip_title_html' do + let(:description) { 'This is an image' } + let(:label_with_html_content) { create(:label, title: title, description: description) } + let(:tooltip) { label_tooltip_title_html(label_with_html_content) } + + context 'when label is unscoped' do + let(:title) { 'test' } + + it 'escapes HTML for display' do + expect(tooltip).to eq('<img src="example.png">This is an image</img>') + end + end + + context 'when label is scoped' do + let(:title) { 'scope::test' } + + it 'includes scoped label tag and escapes HTML for display' do + expect(tooltip).to eq( + "Scoped label
" \ + '<img src="example.png">This is an image</img>') + end + end + end end diff --git a/ee/spec/lib/ee/banzai/filter/references/label_reference_filter_spec.rb b/ee/spec/lib/ee/banzai/filter/references/label_reference_filter_spec.rb index 82ef964e7bd56c850b5e09b0b2a61879877e1167..1e87e7f467f313e01a3ba416bc829b9de8e40753 100644 --- a/ee/spec/lib/ee/banzai/filter/references/label_reference_filter_spec.rb +++ b/ee/spec/lib/ee/banzai/filter/references/label_reference_filter_spec.rb @@ -6,9 +6,9 @@ include FilterSpecHelper let(:project) { create(:project, :public, name: 'sample-project') } - let(:label) { create(:label, name: 'label', project: project) } - let(:scoped_description) { 'xss &lt;svg id="svgId"></svg>' } - let(:scoped_label) { create(:label, name: 'key::value', project: project, description: scoped_description) } + let(:description) { 'xss &lt;svg id="svgId"></svg>' } + let(:label) { create(:label, name: 'label', project: project, description: description) } + let(:scoped_label) { create(:label, name: 'key::value', project: project, description: description) } context 'with scoped labels enabled' do before do @@ -27,17 +27,10 @@ end it "doesn't unescape HTML in the label's title" do - # Note that this is *flawed*: ">" from the input *text* is becoming ">" in the output *HTML*. - # No XSS is possible but we are still corrupting the user's input, albeit less badly than before. - # FIXME. - expect(doc.at_css('.gl-label-scoped a').attr('title')).to include('&lt;svg id="svgId">') - - # The below is what *should* be the case, but LabelsHelper.label_tooltip_title is (incorrectly) sanitising - # instead of safely escaping the output, and LabelsHelper.render_label_text is using html_escape_once (!!), - # which is always bad! - # - # expect(doc.at_css('.gl-label-scoped a').attr('title')).to include('xss <script>alert') - # expect(doc.at_css('.gl-label-scoped a').attr('title')).to include('&<a>lt;svg id=&quot;svgId&quot;&gt;') + # The `title` attribute's DOM value is interpreted as HTML in EE, so we expect it contains the + # description escaped. + expect(doc.at_css('.gl-label-scoped a').attr('title')).to include('xss <script>alert') + expect(doc.at_css('.gl-label-scoped a').attr('title')).to include('&<a>lt;svg id=&quot;svgId&quot;&gt;') end end @@ -48,8 +41,13 @@ expect(doc.css('.gl-label .gl-label-text').map(&:text)).to eq([label.name]) end - it 'renders non-HTML tooltips' do - expect(doc.at_css('.gl-label a').attr('data-html')).to be_nil + it 'renders HTML tooltips' do + expect(doc.at_css('.gl-label a').attr('data-html')).to eq('true') + end + + it "doesn't unescape HTML in the label's title" do + expect(doc.at_css('.gl-label a').attr('title')).to include('xss <script>alert') + expect(doc.at_css('.gl-label a').attr('title')).to include('&<a>lt;svg id=&quot;svgId&quot;&gt;') end end end diff --git a/lib/banzai/filter/references/label_reference_filter.rb b/lib/banzai/filter/references/label_reference_filter.rb index 07ad15247528d04daeabcbe1c13db0f1b8b6ff86..9b89c1ad84e70623357b30f6fcbd4fdaa8e2329c 100644 --- a/lib/banzai/filter/references/label_reference_filter.rb +++ b/lib/banzai/filter/references/label_reference_filter.rb @@ -118,19 +118,17 @@ def url_for_object(label, parent) end def object_link_text(object, matches) - label_suffix = '' + reference = nil parent = project || group if matches[:absolute_path].blank? && (project || full_path_ref?(matches)) project_path = reference_cache.full_project_path(matches[:namespace], matches[:project], matches) parent_from_ref = from_ref_cached(project_path) reference = parent_from_ref.to_human_reference(parent) - - label_suffix = " in #{ERB::Util.html_escape(reference)}" if reference.present? end presenter = object.present(issuable_subject: parent) - LabelsHelper.render_colored_label(presenter, suffix: label_suffix) + LabelsHelper.render_colored_label(presenter, in_reference: reference) end def wrap_link(link, label) @@ -146,6 +144,7 @@ def reference_class(type, tooltip: true) super + ' gl-link gl-label-link' end + # Returns a String containing text. def object_link_title(object, matches) presenter = object.present(issuable_subject: project || group) LabelsHelper.label_tooltip_title(presenter) diff --git a/spec/helpers/labels_helper_spec.rb b/spec/helpers/labels_helper_spec.rb index ed3467eb96c37b86da5ce6c4f0a0471419afc3a6..82ebeaa63c3b7efe353929ab289d4cb239ca73e7 100644 --- a/spec/helpers/labels_helper_spec.rb +++ b/spec/helpers/labels_helper_spec.rb @@ -237,14 +237,14 @@ let(:label_with_html_content) { create(:label, title: 'test', description: html) } context 'tooltip shows description' do - it 'removes HTML' do + it 'leaves HTML untouched' do tooltip = label_tooltip_title(label_with_html_content) - expect(tooltip).to eq('This is an image') + expect(tooltip).to eq(html) end end context 'tooltip shows title' do - it 'shows title' do + it 'returns title' do tooltip = label_tooltip_title(label_with_html_content, tooltip_shows_title: true) expect(tooltip).to eq('test') end diff --git a/spec/lib/banzai/filter/references/label_reference_filter_spec.rb b/spec/lib/banzai/filter/references/label_reference_filter_spec.rb index f66af715a9900047f57dfb8145eed822aba57eed..fd25cc99bf62d214d69ec18602850578c1a8a232 100644 --- a/spec/lib/banzai/filter/references/label_reference_filter_spec.rb +++ b/spec/lib/banzai/filter/references/label_reference_filter_spec.rb @@ -439,13 +439,23 @@ end it 'has valid color' do - expect(result.css('a span').first.attr('style')).to match(/background-color: #00ff00/) + expect(result.css('a span').first.attr('style')).to match('background-color: #00ff00') end it 'has valid link text' do expect(result.css('a').first.text).to eq "#{label.name} in #{project2.full_name}" end + it 'has correct HTML content' do + frag = Nokogiri::HTML.fragment("") + span = frag.children.first + span.content = "#{label.name} " + span << i = frag.document.create_element("i") + i.content = "in #{project2.full_name}" + + expect(result.css('a').first.to_html).to include_html(span.inner_html) + end + it 'has valid text' do expect(result.text).to eq "See #{label.name} in #{project2.full_name}" end