Merge branch 'gh-labels' into 'master'

Allow `?`, or `&` for label names

Closes #18727 

See merge request !4724

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

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)

app/assets/javascripts/labels_select.js.coffee

@@ -32,7 +32,7 @@ class @LabelsSelect
       if issueUpdateURL
         labelHTMLTemplate = _.template(
             '<% _.each(labels, function(label){ %>
-            <a href="<%- ["",issueURLSplit[1], issueURLSplit[2],""].join("/") %>issues?label_name[]=<%- label.title %>">
+            <a href="<%- ["",issueURLSplit[1], issueURLSplit[2],""].join("/") %>issues?label_name[]=<%- encodeURIComponent(label.title) %>">
             <span class="label has-tooltip color-label" title="<%- label.description %>" style="background-color: <%- label.color %>; color: <%- label.text_color %>;">
             <%- label.title %>
             </span>
@@ -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

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
       (?:
         (?<label_id>\d+) | # Integer-based label ID, or
         (?<label_name>
-          [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

app/views/shared/_labels_row.html.haml

 - 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")

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

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(\(<a.+><span.+>\?gfm&amp;</span></a>\.\)))
+    end
+
+    it 'ignores invalid label names' do
+      act = "Label #{Label.reference_prefix}#{label.name.reverse}"
+      exp = "Label #{Label.reference_prefix}&amp;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(\(<a.+><span.+>gfm &amp; references\?</span></a>\.\)))
+    end
+
+    it 'ignores invalid label names' do
+      act = %(Label #{Label.reference_prefix}"#{label.name.reverse}")
+      exp = %(Label #{Label.reference_prefix}"?secnerefer &amp; 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'

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: "<b>test</b>") }
-
-    it "sanitizes title" do
-      expect(label.title).to eq("test")
+  describe '#title' do
+    it 'sanitizes title' do
+      label = described_class.new(title: '<b>foo & bar?</b>')
+      expect(label.title).to eq('foo & bar?')
     end
   end

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

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'])

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