From 357caf7c8b11e79f38756961c5d93426e564767d Mon Sep 17 00:00:00 2001 From: Asherah Connor Date: Mon, 6 Oct 2025 17:39:41 +1100 Subject: [PATCH] Stop unescaping HTML in BaseLabel#title=, #description=, Timebox#title= This affects anything including BaseLabel or Timebox, namely Label, AntiAbuseReports::Label, Milestone, and Iteration. ## Why? Calling `CGI.unescapeHTML` immediately after `Sanitize.clean` is an anti-pattern. Here's an example of `Sanitize.clean` removing something nasty: ```irb [83] pry(main)> Sanitize.clean("hello ") => "hello " ``` What happens if the user enters entities instead? ```irb [84] pry(main)> Sanitize.clean("hello <script>alert('xss')</script>") => "hello <script>alert('xss')</script>" ``` Entities are safe, so this is all OK! But what if we then `CGI.unscapeHTML` it? ```irb [85] pry(main)> CGI.unescapeHTML(Sanitize.clean("hello <script>alert('xss')</script>")) => "hello " ``` Eep! This is **the current state of the application**; we are essentially _not_ sanitising the output, because we're allowing the user to enter anything in entities and then unescaping it *after* sanitisation. Here's an example demonstrating this, but you can easily test it yourself by going to create a new label through the web interface. SQL logs output by the console have been omitted for clarity. ```irb [96] pry(main)> l = Label.first => # [99] pry(main)> l.title = "hello <script>alert('xss')</script>" => "hello <script>alert('xss')</script>" [100] pry(main)> l.save! => true [101] pry(main)> l.reload => # [102] pry(main)> l.title => "hello " ``` The good news is, sanitisation is not the answer here! The user should be able to enter any text as a label or milestone title, and that text should be shown exactly as entered --- no sanitisation needed. We must treat it as text, not HTML. Thankfully, we should be doing this already for the most part, seeing as (as mentioned above) we are already allowing users to enter non-sanitised content here via entity unescaping. --- app/models/concerns/base_label.rb | 21 ++----------------- app/models/concerns/timebox.rb | 9 ++------ ee/spec/models/iteration_spec.rb | 8 ------- spec/models/milestone_spec.rb | 11 ---------- .../concerns/base_label_shared_examples.rb | 10 --------- .../concerns/timebox_shared_examples.rb | 10 +-------- 6 files changed, 5 insertions(+), 64 deletions(-) diff --git a/app/models/concerns/base_label.rb b/app/models/concerns/base_label.rb index b2faa7509e0c48..2153655675086b 100644 --- a/app/models/concerns/base_label.rb +++ b/app/models/concerns/base_label.rb @@ -57,29 +57,12 @@ def text_color color.contrast end - def title=(value) - if value.blank? - super - else - write_attribute(:title, sanitize_value(value)) - end - end - alias_method :name=, :title= - - def description=(value) - if value.blank? - super - else - write_attribute(:description, sanitize_value(value)) - end + def name=(value) + self.title = value end private - def sanitize_value(value) - CGI.unescapeHTML(Sanitize.clean(value.to_s)) - end - def strip_whitespace_from_title self[:title] = title&.strip end diff --git a/app/models/concerns/timebox.rb b/app/models/concerns/timebox.rb index 24150aaae76106..ccef4ad57d53d2 100644 --- a/app/models/concerns/timebox.rb +++ b/app/models/concerns/timebox.rb @@ -126,10 +126,9 @@ def reference_link_text(from = nil) self.class.reference_prefix + self.title end - def title=(value) - write_attribute(:title, sanitize_title(value)) if value.present? + def name=(value) + self.title = value end - alias_method :name=, :title= def timebox_name model_name.singular @@ -172,8 +171,4 @@ def dates_within_4_digits errors.add(:due_date, _("date must not be after 9999-12-31")) end end - - def sanitize_title(value) - CGI.unescape_html(Sanitize.clean(value.to_s)) - end end diff --git a/ee/spec/models/iteration_spec.rb b/ee/spec/models/iteration_spec.rb index b6f31284adeb5a..cad387e2509f7f 100644 --- a/ee/spec/models/iteration_spec.rb +++ b/ee/spec/models/iteration_spec.rb @@ -311,14 +311,6 @@ end end end - - describe 'title' do - subject { build(:iteration, iterations_cadence: iteration_cadence, title: '') } - - it 'sanitizes user intput', :aggregate_failures do - expect(subject.title).to be_blank - end - end end describe 'relations' do diff --git a/spec/models/milestone_spec.rb b/spec/models/milestone_spec.rb index db3fd3d395fc9c..f18838b45bdd8f 100644 --- a/spec/models/milestone_spec.rb +++ b/spec/models/milestone_spec.rb @@ -102,17 +102,6 @@ allow(subject).to receive(:set_iid).and_return(false) end - describe 'title' do - it { is_expected.to validate_presence_of(:title) } - - it 'is invalid if title would be empty after sanitation', :aggregate_failures do - milestone = build(:milestone, project: project, title: '') - - expect(milestone).not_to be_valid - expect(milestone.errors[:title]).to include("can't be blank") - end - end - describe 'milestone_releases' do let(:milestone) { build(:milestone, project: project) } diff --git a/spec/support/shared_examples/models/concerns/base_label_shared_examples.rb b/spec/support/shared_examples/models/concerns/base_label_shared_examples.rb index 895fff8cb71002..bd130f235fca58 100644 --- a/spec/support/shared_examples/models/concerns/base_label_shared_examples.rb +++ b/spec/support/shared_examples/models/concerns/base_label_shared_examples.rb @@ -52,11 +52,6 @@ end describe '#title' do - it 'sanitizes title' do - label = described_class.new(title: 'foo & bar?') - expect(label.title).to eq('foo & bar?') - end - it 'strips title' do label = described_class.new(title: ' label ') label.valid? @@ -66,11 +61,6 @@ end describe '#description' do - it 'sanitizes description' do - label = described_class.new(description: 'foo & bar?') - expect(label.description).to eq('foo & bar?') - end - it 'accepts an empty string' do label = described_class.new(title: 'foo', description: '') label.valid? diff --git a/spec/support/shared_examples/models/concerns/timebox_shared_examples.rb b/spec/support/shared_examples/models/concerns/timebox_shared_examples.rb index 52bedcaa606bc2..f0d1a828bc3710 100644 --- a/spec/support/shared_examples/models/concerns/timebox_shared_examples.rb +++ b/spec/support/shared_examples/models/concerns/timebox_shared_examples.rb @@ -57,15 +57,7 @@ let(:timebox) { create(timebox_type, *timebox_args, title: "foo & bar -> 2.2") } it 'normalizes the title for use as a slug' do - expect(timebox.safe_title).to eq('foo-bar-22') - end - end - - describe "#title" do - let(:timebox) { create(timebox_type, *timebox_args, title: "foo & bar -> 2.2") } - - it "sanitizes title" do - expect(timebox.title).to eq("foo & bar -> 2.2") + expect(timebox.safe_title).to eq('bfoo-bar-22b') end end -- GitLab