Merge branch 'philipcunningham-add-ability-to-sanitize-334653' into 'master'

Add ability to easily sanitize attributes

See merge request gitlab-org/gitlab!68442

app/models/concerns/sanitizable.rb

+# frozen_string_literal: true
+
+# == Sanitizable concern
+#
+# This concern adds HTML sanitization and validation to models. The intention is
+# to help prevent XSS attacks in the event of a by-pass in the frontend
+# sanitizer due to a configuration issue or a vulnerability in the sanitizer.
+# This approach is commonly referred to as defense-in-depth.
+#
+# Example:
+#
+# module Dast
+#   class Profile < ApplicationRecord
+#     include Sanitizable
+#
+#     sanitizes! :name, :description
+
+module Sanitizable
+  extend ActiveSupport::Concern
+
+  class_methods do
+    def sanitize(input)
+      return unless input
+
+      # We return the input unchanged to avoid escaping pre-escaped HTML fragments.
+      # Please see gitlab-org/gitlab#293634 for an example.
+      return input unless input == CGI.unescapeHTML(input.to_s)
+
+      CGI.unescapeHTML(Sanitize.fragment(input))
+    end
+
+    def sanitizes!(*attrs)
+      instance_eval do
+        before_validation do
+          attrs.each do |attr|
+            input = public_send(attr) # rubocop: disable GitlabSecurity/PublicSend
+
+            public_send("#{attr}=", self.class.sanitize(input)) # rubocop: disable GitlabSecurity/PublicSend
+          end
+        end
+
+        validates_each(*attrs) do |record, attr, input|
+          # We reject pre-escaped HTML fragments as invalid to avoid saving them
+          # to the database.
+          unless input.to_s == CGI.unescapeHTML(input.to_s)
+            record.errors.add(attr, 'cannot contain escaped HTML entities')
+          end
+        end
+      end
+    end
+  end
+end

ee/app/models/dast/profile.rb

@@ -2,6 +2,8 @@
 
 module Dast
   class Profile < ApplicationRecord
+    include Sanitizable
+
     self.table_name = 'dast_profiles'
 
     belongs_to :project
@@ -27,6 +29,8 @@ module Dast
 
     delegate :secret_ci_variables, to: :dast_site_profile
 
+    sanitizes! :name, :description
+
     def branch
       return unless project.repository.exists?
 

ee/app/models/dast_scanner_profile.rb

 # frozen_string_literal: true
 
 class DastScannerProfile < ApplicationRecord
+  include Sanitizable
+
   belongs_to :project
 
   validates :project_id, presence: true
@@ -14,6 +16,8 @@ class DastScannerProfile < ApplicationRecord
     active: 2
   }
 
+  sanitizes! :name
+
   def self.names(scanner_profile_ids)
     find(*scanner_profile_ids).pluck(:name)
   rescue ActiveRecord::RecordNotFound

ee/app/models/dast_site_profile.rb

 # frozen_string_literal: true
 
 class DastSiteProfile < ApplicationRecord
+  include Sanitizable
+
   belongs_to :project
   belongs_to :dast_site
 
@@ -25,6 +27,8 @@ class DastSiteProfile < ApplicationRecord
 
   delegate :dast_site_validation, to: :dast_site, allow_nil: true
 
+  sanitizes! :name
+
   def self.names(site_profile_ids)
     find(*site_profile_ids).pluck(:name)
   rescue ActiveRecord::RecordNotFound

ee/spec/models/dast/profile_spec.rb

@@ -7,6 +7,8 @@ RSpec.describe Dast::Profile, type: :model do
 
   subject { create(:dast_profile, project: project) }
 
+  it_behaves_like 'sanitizable', :dast_profile, %i[name description]
+
   describe 'associations' do
     it { is_expected.to belong_to(:project) }
     it { is_expected.to belong_to(:dast_site_profile) }

ee/spec/models/dast_scanner_profile_spec.rb

@@ -5,6 +5,8 @@ require 'spec_helper'
 RSpec.describe DastScannerProfile, type: :model do
   subject { create(:dast_scanner_profile) }
 
+  it_behaves_like 'sanitizable', :dast_scanner_profile, %i[name]
+
   describe 'associations' do
     it { is_expected.to belong_to(:project) }
   end

ee/spec/models/dast_site_profile_spec.rb

@@ -7,6 +7,8 @@ RSpec.describe DastSiteProfile, type: :model do
 
   subject { create(:dast_site_profile, :with_dast_site_validation, project: project) }
 
+  it_behaves_like 'sanitizable', :dast_site_profile, %i[name]
+
   describe 'associations' do
     it { is_expected.to belong_to(:project) }
     it { is_expected.to belong_to(:dast_site) }

spec/models/concerns/sanitizable_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Sanitizable do
+  let_it_be(:klass) do
+    Class.new do
+      include ActiveModel::Model
+      include ActiveModel::Attributes
+      include ActiveModel::Validations
+      include ActiveModel::Validations::Callbacks
+      include Sanitizable
+
+      attribute :id, :integer
+      attribute :name, :string
+      attribute :description, :string
+      attribute :html_body, :string
+
+      sanitizes! :name, :description
+
+      def self.model_name
+        ActiveModel::Name.new(self, nil, 'SomeModel')
+      end
+    end
+  end
+
+  shared_examples 'noop' do
+    it 'has no effect' do
+      expect(subject).to eq(input)
+    end
+  end
+
+  shared_examples 'a sanitizable field' do |field|
+    let(:record) { klass.new(id: 1, name: input, description: input, html_body: input) }
+
+    before do
+      record.valid?
+    end
+
+    subject { record.public_send(field) }
+
+    describe field do
+      context 'when input is nil' do
+        let_it_be(:input) { nil }
+
+        it_behaves_like 'noop'
+      end
+
+      context 'when input does not contain any html' do
+        let_it_be(:input) { 'hello, world!' }
+
+        it_behaves_like 'noop'
+      end
+
+      context 'when input contains html' do
+        let_it_be(:input) { 'hello<script>alert(1)</script>' }
+
+        it 'sanitizes the input' do
+          expect(subject).to eq('hello')
+        end
+
+        context 'when input includes html entities' do
+          let(:input) { '<div>hello&world</div>' }
+
+          it 'does not escape them' do
+            expect(subject).to eq(' hello&world ')
+          end
+        end
+      end
+
+      context 'when input contains pre-escaped html entities' do
+        let_it_be(:input) { '&lt;script&gt;alert(1)&lt;/script&gt;' }
+
+        it_behaves_like 'noop'
+
+        it 'is not valid', :aggregate_failures do
+          expect(record).not_to be_valid
+          expect(record.errors.full_messages).to include('Name cannot contain escaped HTML entities')
+        end
+      end
+    end
+  end
+
+  shared_examples 'a non-sanitizable field' do |field, input|
+    describe field do
+      subject { klass.new(field => input).valid? }
+
+      it 'has no effect' do
+        expect(Sanitize).not_to receive(:fragment)
+
+        subject
+      end
+    end
+  end
+
+  it_behaves_like 'a non-sanitizable field', :id, 1
+  it_behaves_like 'a non-sanitizable field', :html_body, 'hello<script>alert(1)</script>'
+
+  it_behaves_like 'a sanitizable field', :name
+  it_behaves_like 'a sanitizable field', :description
+end

spec/support/shared_examples/models/concerns/sanitizable_shared_examples.rb

+# frozen_string_literal: true
+
+RSpec.shared_examples 'sanitizable' do |factory, fields|
+  let(:attributes) { fields.to_h { |field| [field, input] } }
+
+  it 'includes Sanitizable' do
+    expect(described_class).to include(Sanitizable)
+  end
+
+  fields.each do |field|
+    subject do
+      record = build(factory, attributes)
+      record.valid?
+
+      record.public_send(field)
+    end
+
+    describe "##{field}" do
+      context 'when input includes javascript tags' do
+        let(:input) { 'hello<script>alert(1)</script>' }
+
+        it 'gets sanitized' do
+          expect(subject).to eq('hello')
+        end
+      end
+    end
+
+    describe "##{field} validation" do
+      context 'when input contains pre-escaped html entities' do
+        let_it_be(:input) { '&lt;script&gt;alert(1)&lt;/script&gt;' }
+
+        subject { build(factory, attributes) }
+
+        it 'is not valid', :aggregate_failures do
+          expect(subject).not_to be_valid
+          expect(subject.errors.details[field].flat_map(&:values)).to include('cannot contain escaped HTML entities')
+        end
+      end
+    end
+  end
+end