diff --git a/CHANGELOG b/CHANGELOG index 406dec09e79c3f84bc45d11f5d4796f28f5d16a6..40bccaabb39ff23c95d5ba4d452f6b0a89200bb8 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -42,6 +42,7 @@ v 8.10.0 (unreleased) - Don't garbage collect commits that have related DB records like comments - More descriptive message for git hooks and file locks - Handle custom Git hook result in GitLab UI + - Allow '?', or '&' for label names v 8.9.5 (unreleased) - Improve the request / withdraw access button. !4860 diff --git a/app/assets/javascripts/gfm_auto_complete.js.coffee b/app/assets/javascripts/gfm_auto_complete.js.coffee index b7d040bae85895455afeff3105f3feeeebbd28ce..4a851d9c9fbd626f6f36b997742aafc75fb23e23 100644 --- a/app/assets/javascripts/gfm_auto_complete.js.coffee +++ b/app/assets/javascripts/gfm_auto_complete.js.coffee @@ -190,7 +190,7 @@ GitLab.GfmAutoComplete = callbacks: beforeSave: (merges) -> sanitizeLabelTitle = (title)-> - if /\w+\s+\w+/g.test(title) + if /[\w\?&]+\s+[\w\?&]+/g.test(title) "\"#{sanitize(title)}\"" else sanitize(title) diff --git a/app/assets/javascripts/labels_select.js.coffee b/app/assets/javascripts/labels_select.js.coffee index ce859fedb2de2aa6ea5df34eae2b8bc73e237329..7688609b301dfa5516266e322d006c24e3d76331 100644 --- a/app/assets/javascripts/labels_select.js.coffee +++ b/app/assets/javascripts/labels_select.js.coffee @@ -32,7 +32,7 @@ class @LabelsSelect if issueUpdateURL labelHTMLTemplate = _.template( '<% _.each(labels, function(label){ %> - issues?label_name[]=<%- label.title %>"> + issues?label_name[]=<%- encodeURIComponent(label.title) %>"> <%- label.title %> @@ -261,7 +261,7 @@ class @LabelsSelect $a.attr('data-label-id', label.id) $a.addClass(selectedClass.join(' ')) - .html("#{colorEl} #{_.escape(label.title)}") + .html("#{colorEl} #{label.title}") # Return generated html $li.html($a).prop('outerHTML') @@ -288,7 +288,7 @@ class @LabelsSelect fieldName: $dropdown.data('field-name') id: (label) -> if $dropdown.hasClass("js-filter-submit") and not label.isAny? - _.escape label.title + label.title else label.id diff --git a/app/models/label.rb b/app/models/label.rb index 49c352cc239b811a7f8e772c00b9047d4d75f0a6..dc5586f57566ff5c93de6262e317d559ee887dbe 100644 --- a/app/models/label.rb +++ b/app/models/label.rb @@ -20,10 +20,10 @@ class Label < ActiveRecord::Base validates :color, color: true, allow_blank: false validates :project, presence: true, unless: Proc.new { |service| service.template? } - # Don't allow '?', '&', and ',' for label titles + # Don't allow ',' for label titles validates :title, presence: true, - format: { with: /\A[^&\?,]+\z/ }, + format: { with: /\A[^,]+\z/ }, uniqueness: { scope: :project_id } before_save :nullify_priority @@ -58,8 +58,8 @@ class Label < ActiveRecord::Base (?: (?\d+) | # Integer-based label ID, or (? - [A-Za-z0-9_-]+ | # String-based single-word label title, or - "[^&\?,]+" # String-based multi-word label surrounded in quotes + [A-Za-z0-9_\-\?&]+ | # String-based single-word label title, or + "[^,]+" # String-based multi-word label surrounded in quotes ) ) }x @@ -114,7 +114,7 @@ class Label < ActiveRecord::Base end def title=(value) - write_attribute(:title, Sanitize.clean(value.to_s)) if value.present? + write_attribute(:title, sanitize_title(value)) if value.present? end private @@ -132,4 +132,8 @@ class Label < ActiveRecord::Base def nullify_priority self.priority = nil if priority.blank? end + + def sanitize_title(value) + CGI.unescapeHTML(Sanitize.clean(value.to_s)) + end end diff --git a/app/views/shared/_labels_row.html.haml b/app/views/shared/_labels_row.html.haml index 5507a05f6c1f4690afdc794678f70716a7d40641..dce492352ac6189543ed450268a6a39c507177df 100644 --- a/app/views/shared/_labels_row.html.haml +++ b/app/views/shared/_labels_row.html.haml @@ -1,10 +1,9 @@ - labels.each do |label| - %span.label-row.btn-group{ role: "group", aria: { label: escape_once(label.name) }, style: "color: #{text_color_for_bg(label.color)}" } - = link_to label_filter_path(@project, label, type: controller.controller_name), + %span.label-row.btn-group{ role: "group", aria: { label: label.name }, style: "color: #{text_color_for_bg(label.color)}" } + = link_to label.name, label_filter_path(@project, label, type: controller.controller_name), class: "btn btn-transparent has-tooltip", style: "background-color: #{label.color};", title: escape_once(label.description), - data: { container: "body" } do - = escape_once label.name + data: { container: "body" } %button.btn.btn-transparent.label-remove.js-label-filter-remove{ type: "button", style: "background-color: #{label.color};", data: { label: label.title } } = icon("times") diff --git a/lib/banzai/filter/label_reference_filter.rb b/lib/banzai/filter/label_reference_filter.rb index e4d3f87d0aa77d5e01a86ab0e8d3aa7e59d09e58..e258dc8e2bf017d02bd637d0b6e2a4803bcee2cb 100644 --- a/lib/banzai/filter/label_reference_filter.rb +++ b/lib/banzai/filter/label_reference_filter.rb @@ -13,13 +13,13 @@ module Banzai end def self.references_in(text, pattern = Label.reference_pattern) - text.gsub(pattern) do |match| + unescape_html_entities(text).gsub(pattern) do |match| yield match, $~[:label_id].to_i, $~[:label_name], $~[:project], $~ end end def references_in(text, pattern = Label.reference_pattern) - text.gsub(pattern) do |match| + unescape_html_entities(text).gsub(pattern) do |match| label = find_label($~[:project], $~[:label_id], $~[:label_name]) if label @@ -66,6 +66,10 @@ module Banzai LabelsHelper.render_colored_cross_project_label(object) end end + + def unescape_html_entities(text) + CGI.unescapeHTML(text.to_s) + end end end end diff --git a/spec/lib/banzai/filter/label_reference_filter_spec.rb b/spec/lib/banzai/filter/label_reference_filter_spec.rb index f1064a701d8ca23386b2df446330ca5d8cdd9f09..9e3d2f5825d80b86d9e6e8f985bb242fd93992d5 100644 --- a/spec/lib/banzai/filter/label_reference_filter_spec.rb +++ b/spec/lib/banzai/filter/label_reference_filter_spec.rb @@ -104,6 +104,31 @@ describe Banzai::Filter::LabelReferenceFilter, lib: true do end end + context 'String-based single-word references with special characters' do + let(:label) { create(:label, name: '?gfm&', project: project) } + let(:reference) { "#{Label.reference_prefix}#{label.name}" } + + it 'links to a valid reference' do + doc = reference_filter("See #{reference}") + + expect(doc.css('a').first.attr('href')).to eq urls. + namespace_project_issues_url(project.namespace, project, label_name: label.name) + expect(doc.text).to eq 'See ?gfm&' + end + + it 'links with adjacent text' do + doc = reference_filter("Label (#{reference}.)") + expect(doc.to_html).to match(%r(\(\?gfm&\.\))) + end + + it 'ignores invalid label names' do + act = "Label #{Label.reference_prefix}#{label.name.reverse}" + exp = "Label #{Label.reference_prefix}&mfg?" + + expect(reference_filter(act).to_html).to eq exp + end + end + context 'String-based multi-word references in quotes' do let(:label) { create(:label, name: 'gfm references', project: project) } let(:reference) { label.to_reference(format: :name) } @@ -128,6 +153,31 @@ describe Banzai::Filter::LabelReferenceFilter, lib: true do end end + context 'String-based multi-word references with special characters in quotes' do + let(:label) { create(:label, name: 'gfm & references?', project: project) } + let(:reference) { label.to_reference(format: :name) } + + it 'links to a valid reference' do + doc = reference_filter("See #{reference}") + + expect(doc.css('a').first.attr('href')).to eq urls. + namespace_project_issues_url(project.namespace, project, label_name: label.name) + expect(doc.text).to eq 'See gfm & references?' + end + + it 'links with adjacent text' do + doc = reference_filter("Label (#{reference}.)") + expect(doc.to_html).to match(%r(\(gfm & references\?\.\))) + end + + it 'ignores invalid label names' do + act = %(Label #{Label.reference_prefix}"#{label.name.reverse}") + exp = %(Label #{Label.reference_prefix}"?secnerefer & mfg\") + + expect(reference_filter(act).to_html).to eq exp + end + end + describe 'edge cases' do it 'gracefully handles non-references matching the pattern' do exp = act = '(format nil "~0f" 3.0) ; 3.0' diff --git a/spec/models/label_spec.rb b/spec/models/label_spec.rb index dad2628651b480b74c98cdf759ac9df9d7159b44..f37f44a608e53c3aac5fa28a32a2e661f912afd5 100644 --- a/spec/models/label_spec.rb +++ b/spec/models/label_spec.rb @@ -32,21 +32,20 @@ describe Label, models: true do it 'should validate title' do expect(label).not_to allow_value('G,ITLAB').for(:title) - expect(label).not_to allow_value('G?ITLAB').for(:title) - expect(label).not_to allow_value('G&ITLAB').for(:title) expect(label).not_to allow_value('').for(:title) expect(label).to allow_value('GITLAB').for(:title) expect(label).to allow_value('gitlab').for(:title) + expect(label).to allow_value('G?ITLAB').for(:title) + expect(label).to allow_value('G&ITLAB').for(:title) expect(label).to allow_value("customer's request").for(:title) end end - describe "#title" do - let(:label) { create(:label, title: "test") } - - it "sanitizes title" do - expect(label.title).to eq("test") + describe '#title' do + it 'sanitizes title' do + label = described_class.new(title: 'foo & bar?') + expect(label.title).to eq('foo & bar?') end end diff --git a/spec/requests/api/issues_spec.rb b/spec/requests/api/issues_spec.rb index 2cf130df328fc8e8b4c9d1fc6625eb26730fbdf2..6adccb4ebae9348f8d7e928b602a70b68267b2ef 100644 --- a/spec/requests/api/issues_spec.rb +++ b/spec/requests/api/issues_spec.rb @@ -482,12 +482,16 @@ describe API::API, api: true do expect(response).to have_http_status(400) end - it 'should return 400 on invalid label names' do + it 'should allow special label names' do post api("/projects/#{project.id}/issues", user), title: 'new issue', - labels: 'label, ?' - expect(response).to have_http_status(400) - expect(json_response['message']['labels']['?']['title']).to eq(['is invalid']) + labels: 'label, label?, label&foo, ?, &' + expect(response.status).to eq(201) + expect(json_response['labels']).to include 'label' + expect(json_response['labels']).to include 'label?' + expect(json_response['labels']).to include 'label&foo' + expect(json_response['labels']).to include '?' + expect(json_response['labels']).to include '&' end it 'should return 400 if title is too long' do @@ -557,12 +561,17 @@ describe API::API, api: true do expect(response).to have_http_status(404) end - it 'should return 400 on invalid label names' do + it 'should allow special label names' do put api("/projects/#{project.id}/issues/#{issue.id}", user), title: 'updated title', - labels: 'label, ?' - expect(response).to have_http_status(400) - expect(json_response['message']['labels']['?']['title']).to eq(['is invalid']) + labels: 'label, label?, label&foo, ?, &' + + expect(response.status).to eq(200) + expect(json_response['labels']).to include 'label' + expect(json_response['labels']).to include 'label?' + expect(json_response['labels']).to include 'label&foo' + expect(json_response['labels']).to include '?' + expect(json_response['labels']).to include '&' end context 'confidential issues' do @@ -627,21 +636,18 @@ describe API::API, api: true do expect(json_response['labels']).to include 'bar' end - it 'should return 400 on invalid label names' do - put api("/projects/#{project.id}/issues/#{issue.id}", user), - labels: 'label, ?' - expect(response).to have_http_status(400) - expect(json_response['message']['labels']['?']['title']).to eq(['is invalid']) - end - it 'should allow special label names' do put api("/projects/#{project.id}/issues/#{issue.id}", user), - labels: 'label:foo, label-bar,label_bar,label/bar' - expect(response).to have_http_status(200) + labels: 'label:foo, label-bar,label_bar,label/bar,label?bar,label&bar,?,&' + expect(response.status).to eq(200) expect(json_response['labels']).to include 'label:foo' expect(json_response['labels']).to include 'label-bar' expect(json_response['labels']).to include 'label_bar' expect(json_response['labels']).to include 'label/bar' + expect(json_response['labels']).to include 'label?bar' + expect(json_response['labels']).to include 'label&bar' + expect(json_response['labels']).to include '?' + expect(json_response['labels']).to include '&' end it 'should return 400 if title is too long' do diff --git a/spec/requests/api/labels_spec.rb b/spec/requests/api/labels_spec.rb index 0404cf31ff7c6b356c9ccec157e43e386eca5b50..63636b4a1b61a8cb0f1b270499ca34cc35ba7371 100644 --- a/spec/requests/api/labels_spec.rb +++ b/spec/requests/api/labels_spec.rb @@ -35,10 +35,10 @@ describe API::API, api: true do it 'should return created label when only required params' do post api("/projects/#{project.id}/labels", user), - name: 'Foo', + name: 'Foo & Bar', color: '#FFAABB' - expect(response).to have_http_status(201) - expect(json_response['name']).to eq('Foo') + expect(response.status).to eq(201) + expect(json_response['name']).to eq('Foo & Bar') expect(json_response['color']).to eq('#FFAABB') expect(json_response['description']).to be_nil end @@ -71,7 +71,7 @@ describe API::API, api: true do it 'should return 400 for invalid name' do post api("/projects/#{project.id}/labels", user), - name: '?', + name: ',', color: '#FFAABB' expect(response).to have_http_status(400) expect(json_response['message']['title']).to eq(['is invalid']) @@ -167,7 +167,7 @@ describe API::API, api: true do it 'should return 400 for invalid name' do put api("/projects/#{project.id}/labels", user), name: 'label1', - new_name: '?', + new_name: ',', color: '#FFFFFF' expect(response).to have_http_status(400) expect(json_response['message']['title']).to eq(['is invalid']) diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb index 61e897edf8751afa22265df08cbe2eaed03125bb..5d81844fb84bd3ba2d6b64e46f188b888b53b33b 100644 --- a/spec/requests/api/merge_requests_spec.rb +++ b/spec/requests/api/merge_requests_spec.rb @@ -243,17 +243,19 @@ describe API::API, api: true do expect(response).to have_http_status(400) end - it 'should return 400 on invalid label names' do + it 'should allow special label names' do post api("/projects/#{project.id}/merge_requests", user), title: 'Test merge_request', source_branch: 'markdown', target_branch: 'master', author: user, - labels: 'label, ?' - expect(response).to have_http_status(400) - expect(json_response['message']['labels']['?']['title']).to eq( - ['is invalid'] - ) + labels: 'label, label?, label&foo, ?, &' + expect(response.status).to eq(201) + expect(json_response['labels']).to include 'label' + expect(json_response['labels']).to include 'label?' + expect(json_response['labels']).to include 'label&foo' + expect(json_response['labels']).to include '?' + expect(json_response['labels']).to include '&' end context 'with existing MR' do @@ -492,13 +494,17 @@ describe API::API, api: true do expect(json_response['target_branch']).to eq('wiki') end - it 'should return 400 on invalid label names' do + it 'should allow special label names' do put api("/projects/#{project.id}/merge_requests/#{merge_request.id}", user), title: 'new issue', - labels: 'label, ?' - expect(response).to have_http_status(400) - expect(json_response['message']['labels']['?']['title']).to eq(['is invalid']) + labels: 'label, label?, label&foo, ?, &' + expect(response.status).to eq(200) + expect(json_response['labels']).to include 'label' + expect(json_response['labels']).to include 'label?' + expect(json_response['labels']).to include 'label&foo' + expect(json_response['labels']).to include '?' + expect(json_response['labels']).to include '&' end end