Prevent XSS in group name validations

GitLab currently uses a regex to validate group names.
It has proved difficult to prevent XSS problems. In trying to
chase XSS issues we've tightened the regex and don't allow
some completely benign characters on their own, such as
parentheses. This results in a worse user experience and may not
really protect from XSS. Instead, this now uses the
Rails::Html::FullSanitizer to validate group names.

app/models/group.rb

@@ -73,9 +73,12 @@ class Group < Namespace
   validates :variables, variable_duplicates: true
 
   validates :two_factor_grace_period, presence: true, numericality: { greater_than_or_equal_to: 0 }
+
   validates :name,
-    format: { with: Gitlab::Regex.group_name_regex,
-              message: Gitlab::Regex.group_name_regex_message }, if: :name_changed?
+            html_safety: true,
+            format: { with: Gitlab::Regex.group_name_regex,
+                      message: Gitlab::Regex.group_name_regex_message },
+            if: :name_changed?
 
   add_authentication_token_field :runners_token, encrypted: -> { Feature.enabled?(:groups_tokens_optional_encryption, default_enabled: true) ? :optional : :required }
 

app/validators/html_safety_validator.rb

+# frozen_string_literal: true
+
+# HtmlSafetyValidator
+#
+# Validates that a value does not contain HTML
+# or other unsafe content that could lead to XSS.
+# Relies on Rails HTML Sanitizer:
+# https://github.com/rails/rails-html-sanitizer
+#
+# Example:
+#
+#   class Group < ActiveRecord::Base
+#     validates :name, presence: true, html_safety: true
+#   end
+#
+class HtmlSafetyValidator < ActiveModel::EachValidator
+  def validate_each(record, attribute, value)
+    return if value.blank? || safe_value?(value)
+
+    record.errors.add(attribute, self.class.error_message)
+  end
+
+  def self.error_message
+    _("cannot contain HTML/XML tags, including any word between angle brackets (<,>).")
+  end
+
+  private
+
+  # The `FullSanitizer` encodes ampersands as the HTML entity name.
+  # This isn't particularly necessary for preventing XSS so the ampersand
+  # is pre-encoded to avoid it being flagged in the comparison.
+  def safe_value?(text)
+    pre_encoded_text = text.gsub('&', '&amp;')
+    Rails::Html::FullSanitizer.new.sanitize(pre_encoded_text) == pre_encoded_text
+  end
+end

changelogs/unreleased/security-dblessing-sanitize-group-names.yml

+---
+title: Prevent XSS in group name validations
+merge_request:
+author:
+type: security

ee/spec/controllers/subscriptions_controller_spec.rb

@@ -155,7 +155,7 @@ RSpec.describe SubscriptionsController do
             group.save
             subject
 
-            expect(response.body).to include({ name: [Gitlab::Regex.group_name_regex_message] }.to_json)
+            expect(Gitlab::Json.parse(response.body)['name']).to match_array([Gitlab::Regex.group_name_regex_message, HtmlSafetyValidator.error_message])
           end
         end
       end

locale/gitlab.pot

@@ -26497,6 +26497,9 @@ msgstr ""
 msgid "cannot block others"
 msgstr ""
 
+msgid "cannot contain HTML/XML tags, including any word between angle brackets (<,>)."
+msgstr ""
+
 msgid "cannot include leading slash or directory traversal."
 msgstr ""
 

spec/validators/html_safety_validator_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe HtmlSafetyValidator do
+  let(:validator) { described_class.new(attributes: [:name]) }
+  let(:group) { build(:group) }
+
+  def validate(value)
+    validator.validate_each(group, :name, value)
+  end
+
+  it 'adds an error when a script is included in the name' do
+    validate('My group <script>evil_script</script>')
+
+    expect(group.errors[:name]).to eq([HtmlSafetyValidator.error_message])
+  end
+
+  it 'does not add an error when an ampersand is included in the name' do
+    validate('Group with 1 & 2')
+
+    expect(group.errors).to be_empty
+  end
+end