Unescape and sanitize protected tag name on create and update

The frontend escapes the names of tags when listing the names in
the dropdown. It also submits the escaped name as the tag to
protect when creating a protected tag.

To fix it, we unescape and sanitize the name so it matches the
appropriate tag name to be protected.

Changelog: fixed

app/services/concerns/protected_ref_name_sanitizer.rb

+# frozen_string_literal: true
+
+module ProtectedRefNameSanitizer
+  def sanitize_name(name)
+    name = CGI.unescapeHTML(name)
+    name = Sanitize.fragment(name)
+
+    # Sanitize.fragment escapes HTML chars, so unescape again to allow names
+    # like `feature->master`
+    CGI.unescapeHTML(name)
+  end
+end

app/services/protected_branches/base_service.rb

@@ -2,6 +2,8 @@
 
 module ProtectedBranches
   class BaseService < ::BaseService
+    include ProtectedRefNameSanitizer
+
     # current_user - The user that performs the action
     # params - A hash of parameters
     def initialize(project, current_user = nil, params = {})
@@ -14,22 +16,13 @@ module ProtectedBranches
       # overridden in EE::ProtectedBranches module
     end
 
+    private
+
     def filtered_params
       return unless params
 
-      params[:name] = sanitize_branch_name(params[:name]) if params[:name].present?
+      params[:name] = sanitize_name(params[:name]) if params[:name].present?
       params
     end
-
-    private
-
-    def sanitize_branch_name(name)
-      name = CGI.unescapeHTML(name)
-      name = Sanitize.fragment(name)
-
-      # Sanitize.fragment escapes HTML chars, so unescape again to allow names
-      # like `feature->master`
-      CGI.unescapeHTML(name)
-    end
   end
 end

app/services/protected_tags/base_service.rb

+# frozen_string_literal: true
+
+module ProtectedTags
+  class BaseService < ::BaseService
+    include ProtectedRefNameSanitizer
+
+    private
+
+    def filtered_params
+      return unless params
+
+      params[:name] = sanitize_name(params[:name]) if params[:name].present?
+      params
+    end
+  end
+end

app/services/protected_tags/create_service.rb

 # frozen_string_literal: true
 
 module ProtectedTags
-  class CreateService < BaseService
+  class CreateService < ProtectedTags::BaseService
     attr_reader :protected_tag
 
     def execute
       raise Gitlab::Access::AccessDeniedError unless can?(current_user, :admin_project, project)
 
-      project.protected_tags.create(params)
+      project.protected_tags.create(filtered_params)
     end
   end
 end

app/services/protected_tags/update_service.rb

 # frozen_string_literal: true
 
 module ProtectedTags
-  class UpdateService < BaseService
+  class UpdateService < ProtectedTags::BaseService
     def execute(protected_tag)
       raise Gitlab::Access::AccessDeniedError unless can?(current_user, :admin_project, project)
 
-      protected_tag.update(params)
+      protected_tag.update(filtered_params)
       protected_tag
     end
   end

spec/services/protected_tags/create_service_spec.rb

@@ -7,17 +7,54 @@ RSpec.describe ProtectedTags::CreateService do
   let(:user) { project.owner }
   let(:params) do
     {
-      name: 'master',
+      name: name,
       create_access_levels_attributes: [{ access_level: Gitlab::Access::MAINTAINER }]
     }
   end
 
   describe '#execute' do
+    let(:name) { 'tag' }
+
     subject(:service) { described_class.new(project, user, params) }
 
     it 'creates a new protected tag' do
       expect { service.execute }.to change(ProtectedTag, :count).by(1)
       expect(project.protected_tags.last.create_access_levels.map(&:access_level)).to eq([Gitlab::Access::MAINTAINER])
     end
+
+    context 'when name has escaped HTML' do
+      let(:name) { 'tag->test' }
+
+      it 'creates the new protected tag matching the unescaped version' do
+        expect { service.execute }.to change(ProtectedTag, :count).by(1)
+        expect(project.protected_tags.last.name).to eq('tag->test')
+      end
+
+      context 'and name contains HTML tags' do
+        let(:name) { '<b>tag</b>' }
+
+        it 'creates the new protected tag with sanitized name' do
+          expect { service.execute }.to change(ProtectedTag, :count).by(1)
+          expect(project.protected_tags.last.name).to eq('tag')
+        end
+
+        context 'and contains unsafe HTML' do
+          let(:name) { '<script>alert('foo');</script>' }
+
+          it 'does not create the new protected tag' do
+            expect { service.execute }.not_to change(ProtectedTag, :count)
+          end
+        end
+      end
+
+      context 'when name contains unescaped HTML tags' do
+        let(:name) { '<b>tag</b>' }
+
+        it 'creates the new protected tag with sanitized name' do
+          expect { service.execute }.to change(ProtectedTag, :count).by(1)
+          expect(project.protected_tags.last.name).to eq('tag')
+        end
+      end
+    end
   end
 end

spec/services/protected_tags/update_service_spec.rb

@@ -6,17 +6,50 @@ RSpec.describe ProtectedTags::UpdateService do
   let(:protected_tag) { create(:protected_tag) }
   let(:project) { protected_tag.project }
   let(:user) { project.owner }
-  let(:params) { { name: 'new protected tag name' } }
+  let(:params) { { name: new_name } }
 
   describe '#execute' do
+    let(:new_name) { 'new protected tag name' }
+    let(:result) { service.execute(protected_tag) }
+
     subject(:service) { described_class.new(project, user, params) }
 
     it 'updates a protected tag' do
-      result = service.execute(protected_tag)
-
       expect(result.reload.name).to eq(params[:name])
     end
 
+    context 'when name has escaped HTML' do
+      let(:new_name) { 'tag->test' }
+
+      it 'updates protected tag name with unescaped HTML' do
+        expect(result.reload.name).to eq('tag->test')
+      end
+
+      context 'and name contains HTML tags' do
+        let(:new_name) { '<b>tag</b>' }
+
+        it 'updates protected tag name with sanitized name' do
+          expect(result.reload.name).to eq('tag')
+        end
+
+        context 'and contains unsafe HTML' do
+          let(:new_name) { '<script>alert('foo');</script>' }
+
+          it 'does not update the protected tag' do
+            expect(result.reload.name).to eq(protected_tag.name)
+          end
+        end
+      end
+    end
+
+    context 'when name contains unescaped HTML tags' do
+      let(:new_name) { '<b>tag</b>' }
+
+      it 'updates protected tag name with sanitized name' do
+        expect(result.reload.name).to eq('tag')
+      end
+    end
+
     context 'without admin_project permissions' do
       let(:user) { create(:user) }