diff --git a/app/models/concerns/sanitizable.rb b/app/models/concerns/sanitizable.rb new file mode 100644 index 0000000000000000000000000000000000000000..05756beb404058e13d8db08f2359e829bfa99a7e --- /dev/null +++ b/app/models/concerns/sanitizable.rb @@ -0,0 +1,52 @@ +# 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 diff --git a/ee/app/models/dast/profile.rb b/ee/app/models/dast/profile.rb index 542f71bcc164b9981f918c7a6992f463b2357431..e2b0e9e811bfe8cbdc3dc0a8d0b4808bc7a49b57 100644 --- a/ee/app/models/dast/profile.rb +++ b/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 @@ class Profile < ApplicationRecord delegate :secret_ci_variables, to: :dast_site_profile + sanitizes! :name, :description + def branch return unless project.repository.exists? diff --git a/ee/app/models/dast_scanner_profile.rb b/ee/app/models/dast_scanner_profile.rb index 17373cb23672dc3ea4b07b913357abe48cfc821b..d102efebb5c1af375504814c9e309704d1cb754c 100644 --- a/ee/app/models/dast_scanner_profile.rb +++ b/ee/app/models/dast_scanner_profile.rb @@ -1,6 +1,8 @@ # 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 diff --git a/ee/app/models/dast_site_profile.rb b/ee/app/models/dast_site_profile.rb index 53010356c1c3dba333a85013dd5fb3becc41444f..de12d54aed21b1cd57171c961ed83bc52610e7c4 100644 --- a/ee/app/models/dast_site_profile.rb +++ b/ee/app/models/dast_site_profile.rb @@ -1,6 +1,8 @@ # 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 diff --git a/ee/spec/models/dast/profile_spec.rb b/ee/spec/models/dast/profile_spec.rb index a76216f431f5490190a55c92d7818a73d5d0c166..72a017829a492866d9030529da10d5127246ca3b 100644 --- a/ee/spec/models/dast/profile_spec.rb +++ b/ee/spec/models/dast/profile_spec.rb @@ -7,6 +7,8 @@ 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) } diff --git a/ee/spec/models/dast_scanner_profile_spec.rb b/ee/spec/models/dast_scanner_profile_spec.rb index 5ae1f03b5277ca643c112e2d7c4589dbd8b7783c..d2f3e4ea5002202dfe7d9d4262dcacdfa53d8837 100644 --- a/ee/spec/models/dast_scanner_profile_spec.rb +++ b/ee/spec/models/dast_scanner_profile_spec.rb @@ -5,6 +5,8 @@ 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 diff --git a/ee/spec/models/dast_site_profile_spec.rb b/ee/spec/models/dast_site_profile_spec.rb index 69f2e62c8a8edf5891b41012e1da29a98e093ebb..8a5e69c7d873ed07acb8dc87b3ef4260eee39552 100644 --- a/ee/spec/models/dast_site_profile_spec.rb +++ b/ee/spec/models/dast_site_profile_spec.rb @@ -7,6 +7,8 @@ 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) } diff --git a/spec/models/concerns/sanitizable_spec.rb b/spec/models/concerns/sanitizable_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..4a1d463d666a66ac10d53eccc2950161f65bef8f --- /dev/null +++ b/spec/models/concerns/sanitizable_spec.rb @@ -0,0 +1,101 @@ +# 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) { '<script>alert(1)</script>' } + + 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 diff --git a/spec/support/shared_examples/models/concerns/sanitizable_shared_examples.rb b/spec/support/shared_examples/models/concerns/sanitizable_shared_examples.rb new file mode 100644 index 0000000000000000000000000000000000000000..ed94a71892d291b7a126d95ce2b9ce2725fb1fe7 --- /dev/null +++ b/spec/support/shared_examples/models/concerns/sanitizable_shared_examples.rb @@ -0,0 +1,41 @@ +# 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) { '<script>alert(1)</script>' } + + 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