From 7b5d9cee2859e28a2b18ee68cd1ee5e2815f8172 Mon Sep 17 00:00:00 2001 From: Asherah Connor Date: Mon, 6 Oct 2025 17:39:41 +1100 Subject: [PATCH 1/6] 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 From 734bd3fe93de04c2c42bd3cf130b981d726ed45e Mon Sep 17 00:00:00 2001 From: Asherah Connor Date: Thu, 6 Nov 2025 11:58:53 +1100 Subject: [PATCH 2/6] Add back specs which assert and motivate the new behaviour --- ee/spec/models/iteration_spec.rb | 16 +++++++++++++++ spec/models/milestone_spec.rb | 16 +++++++++++++++ .../concerns/base_label_shared_examples.rb | 20 +++++++++++++++++++ .../concerns/timebox_shared_examples.rb | 16 +++++++++++++++ 4 files changed, 68 insertions(+) diff --git a/ee/spec/models/iteration_spec.rb b/ee/spec/models/iteration_spec.rb index cad387e2509f7f..1e74e73429d5a4 100644 --- a/ee/spec/models/iteration_spec.rb +++ b/ee/spec/models/iteration_spec.rb @@ -311,6 +311,22 @@ end end end + + describe 'title' do + subject { build(:iteration, iterations_cadence: iteration_cadence, title: input_title) } + + context 'when input contains HTML entities and HTML tags' do + let(:input_title) { '<hello>' } + + it 'leaves the input title unchanged' do + # This field is not ever to be treated as HTML; it is text, never unescaped or sanitised, + # and is always escaped when inserted into HTML directly. + # If an XSS occurs in future which would lead you to wanting to "fix" this spec, please + # instead fix it at the point of display, not by corrupting user input! + expect(subject.title).to eq(input_title) + end + end + end end describe 'relations' do diff --git a/spec/models/milestone_spec.rb b/spec/models/milestone_spec.rb index f18838b45bdd8f..bd885e1d2fef09 100644 --- a/spec/models/milestone_spec.rb +++ b/spec/models/milestone_spec.rb @@ -102,6 +102,22 @@ allow(subject).to receive(:set_iid).and_return(false) end + describe 'title' do + subject { build(:milestone, project: project, title: input_title) } + + context 'when input contains HTML entities and HTML tags' do + let(:input_title) { '<hello>' } + + it 'leaves the input title unchanged' do + # This field is not ever to be treated as HTML; it is text, never unescaped or sanitised, + # and is always escaped when inserted into HTML directly. + # If an XSS occurs in future which would lead you to wanting to "fix" this spec, please + # instead fix it at the point of display, not by corrupting user input! + expect(subject.title).to eq(input_title) + end + 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 bd130f235fca58..7a7e54d04eb234 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 @@ -1,6 +1,22 @@ # frozen_string_literal: true RSpec.shared_examples 'BaseLabel' do |factory_name: :label| + shared_examples_for 'text input field' do |field_name| + subject { described_class.new(field_name => input) } + + context 'when input contains HTML entities and HTML tags' do + let(:input) { '<hello>' } + + it 'leaves the input unchanged' do + # This field is not ever to be treated as HTML; it is text, never unescaped or sanitised, + # and is always escaped when inserted into HTML directly. + # If an XSS occurs in future which would lead you to wanting to "fix" this spec, please + # instead fix it at the point of display, not by corrupting user input! + expect(subject.public_send(field_name)).to eq(input) + end + end + end + describe 'validation' do it 'validates color code' do is_expected.not_to allow_value('G-ITLAB').for(:color) @@ -58,6 +74,8 @@ expect(label.title).to eq('label') end + + it_behaves_like 'text input field', :title end describe '#description' do @@ -67,6 +85,8 @@ expect(label.errors[:description]).to be_empty end + + it_behaves_like 'text input field', :description end describe '.search' do 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 f0d1a828bc3710..9556f3e8b0cdec 100644 --- a/spec/support/shared_examples/models/concerns/timebox_shared_examples.rb +++ b/spec/support/shared_examples/models/concerns/timebox_shared_examples.rb @@ -61,6 +61,22 @@ end end + describe '#title' do + subject { create(timebox_type, *timebox_args, title: input_title) } + + context 'when input contains HTML entities and HTML tags' do + let(:input_title) { '<hello>' } + + it 'leaves the input unchanged' do + # This field is not ever to be treated as HTML; it is text, never unescaped or sanitised, + # and is always escaped when inserted into HTML directly. + # If an XSS occurs in future which would lead you to wanting to "fix" this spec, please + # instead fix it at the point of display, not by corrupting user input! + expect(subject.title).to eq(input_title) + end + end + end + describe '#to_ability_name' do it 'returns timebox' do timebox = build(timebox_type, *timebox_args) -- GitLab From 429ced7fa4ad96cf0a74d72ded1aacaf3f291bd6 Mon Sep 17 00:00:00 2001 From: Asherah Connor Date: Thu, 6 Nov 2025 11:58:53 +1100 Subject: [PATCH 3/6] Adjust milestone controller specs for new behaviour To trigger a validation error, we now need to actually give it a blank title. --- spec/controllers/groups/milestones_controller_spec.rb | 4 +--- spec/controllers/projects/milestones_controller_spec.rb | 9 ++++----- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/spec/controllers/groups/milestones_controller_spec.rb b/spec/controllers/groups/milestones_controller_spec.rb index 8b34c6e4a43a24..f1f5ff961b6dc7 100644 --- a/spec/controllers/groups/milestones_controller_spec.rb +++ b/spec/controllers/groups/milestones_controller_spec.rb @@ -313,9 +313,7 @@ end it 'responds unprocessable_entity (422) with error data' do - # Note: This assignment ensures and triggers a validation error when updating the milestone. - # Same approach used in spec/models/milestone_spec.rb . - milestone_params[:title] = '' + milestone_params[:title] = '' subject diff --git a/spec/controllers/projects/milestones_controller_spec.rb b/spec/controllers/projects/milestones_controller_spec.rb index 7caa0c0bd57f23..7a1ac7580f112c 100644 --- a/spec/controllers/projects/milestones_controller_spec.rb +++ b/spec/controllers/projects/milestones_controller_spec.rb @@ -233,9 +233,7 @@ def render_index(project:, page:, search_title: '') end it 'responds unprocessable_entity (422) with error data' do - # Note: This assignment ensures and triggers a validation error when updating the milestone. - # Same approach used in spec/models/milestone_spec.rb . - milestone_params[:title] = '' + milestone_params[:title] = '' subject @@ -346,11 +344,12 @@ def render_index(project:, page:, search_title: '') end it 'renders milestone name without parsing it as HTML' do - milestone.update!(name: 'CCC<img src=x onerror=alert(document.domain)>') + name = 'CCC' + milestone.update!(name: name) post :promote, params: { namespace_id: project.namespace.id, project_id: project.id, id: milestone.iid } - expect(flash[:notice]).to eq("CCC promoted to group milestone.") + expect(flash[:notice]).to eq("#{CGI.escapeHTML(name)} promoted to group milestone.") end end -- GitLab From b7eb7767e4753553572917742430a84f64736112 Mon Sep 17 00:00:00 2001 From: Asherah Connor Date: Thu, 6 Nov 2025 11:58:53 +1100 Subject: [PATCH 4/6] Label controller spec is doing the right thing --- spec/controllers/projects/labels_controller_spec.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/spec/controllers/projects/labels_controller_spec.rb b/spec/controllers/projects/labels_controller_spec.rb index 40feb6e134877a..dc5423528b9f6a 100644 --- a/spec/controllers/projects/labels_controller_spec.rb +++ b/spec/controllers/projects/labels_controller_spec.rb @@ -222,11 +222,12 @@ def toggle_subscription(label) end it 'renders label name without parsing it as HTML' do - label_1.update!(name: 'CCC<img src=x onerror=alert(document.domain)>') + name = 'CCC' + label_1.update!(name: name) post :promote, params: { namespace_id: project.namespace.to_param, project_id: project, id: label_1.to_param } - expect(flash[:notice]).to eq("CCC<img src=x onerror=alert(document.domain)> promoted to group label.") + expect(flash[:notice]).to eq("#{CGI.escapeHTML(name)} promoted to group label.") end context 'service raising InvalidRecord' do -- GitLab From 5c88feeea1635766d810caccf0cd00385a3b2873 Mon Sep 17 00:00:00 2001 From: Asherah Connor Date: Thu, 6 Nov 2025 11:58:53 +1100 Subject: [PATCH 5/6] We no longer expect this to get unescaped automatically (!!) --- spec/features/issues/issue_sidebar_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/features/issues/issue_sidebar_spec.rb b/spec/features/issues/issue_sidebar_spec.rb index 9a1a6a9cf2f1a9..6c6dc64a99e990 100644 --- a/spec/features/issues/issue_sidebar_spec.rb +++ b/spec/features/issues/issue_sidebar_spec.rb @@ -147,7 +147,7 @@ let_it_be(:development) { create(:group_label, group: group, name: 'Development') } let_it_be(:stretch) { create(:label, project: project, name: 'Stretch') } let_it_be(:xss_label) do - create(:label, project: project, title: '<script>alert("xss");</script>') + create(:label, project: project, title: '') end it 'shows labels list in the dropdown' do -- GitLab From be9152638b41807a9c6ad6200a88aad9373a03c2 Mon Sep 17 00:00:00 2001 From: Asherah Connor Date: Thu, 6 Nov 2025 11:58:53 +1100 Subject: [PATCH 6/6] Clean up the specs which led to this MR --- .../iteration_reference_filter_spec.rb | 13 +++++------ .../references/label_reference_filter_spec.rb | 12 +++++++++- .../references/label_reference_filter_spec.rb | 22 +++++++++---------- .../milestone_reference_filter_spec.rb | 18 +++++++-------- .../reference_filter_shared_examples.rb | 9 ++++---- 5 files changed, 40 insertions(+), 34 deletions(-) diff --git a/ee/spec/lib/banzai/filter/references/iteration_reference_filter_spec.rb b/ee/spec/lib/banzai/filter/references/iteration_reference_filter_spec.rb index 27c338e245d40c..0e076953410db1 100644 --- a/ee/spec/lib/banzai/filter/references/iteration_reference_filter_spec.rb +++ b/ee/spec/lib/banzai/filter/references/iteration_reference_filter_spec.rb @@ -333,14 +333,13 @@ end it_behaves_like 'a reference which does not unescape its content in data-original' do - let(:context) { { project: nil, group: group } } - let(:iteration_title) { "x<script>alert('xss');</script>" } - let(:resource) { create(:iteration, title: iteration_title, group: group) } - let(:reference) { %(#{resource.class.reference_prefix}"#{iteration_title}") } - - # This is probably bad. - let(:expected_resource_title) { "x" } + let(:context) { { project: nil, group: group } } + let(:iteration_title) { "x" } + let(:iteration_title_escaped) { "x<script>alert('xss');</script>" } + let(:resource) { create(:iteration, title: iteration_title, group: group) } + let(:reference) { %(#{resource.class.reference_prefix}"#{iteration_title_escaped}") } + let(:expected_resource_title) { iteration_title } let(:expected_href) { urls.iteration_url(resource) } let(:expected_replacement_text) { resource.display_text } end diff --git a/ee/spec/lib/ee/banzai/filter/references/label_reference_filter_spec.rb b/ee/spec/lib/ee/banzai/filter/references/label_reference_filter_spec.rb index fc1aa60fd3904c..82ef964e7bd56c 100644 --- a/ee/spec/lib/ee/banzai/filter/references/label_reference_filter_spec.rb +++ b/ee/spec/lib/ee/banzai/filter/references/label_reference_filter_spec.rb @@ -27,7 +27,17 @@ end it "doesn't unescape HTML in the label's title" do - expect(doc.at_css('.gl-label-scoped a').attr('title')).to include('xss <svg id="svgId">') + # Note that this is *flawed*: ">" from the input *text* is becoming ">" in the output *HTML*. + # No XSS is possible but we are still corrupting the user's input, albeit less badly than before. + # FIXME. + expect(doc.at_css('.gl-label-scoped a').attr('title')).to include('&lt;svg id="svgId">') + + # The below is what *should* be the case, but LabelsHelper.label_tooltip_title is (incorrectly) sanitising + # instead of safely escaping the output, and LabelsHelper.render_label_text is using html_escape_once (!!), + # which is always bad! + # + # expect(doc.at_css('.gl-label-scoped a').attr('title')).to include('xss <script>alert') + # expect(doc.at_css('.gl-label-scoped a').attr('title')).to include('&<a>lt;svg id=&quot;svgId&quot;&gt;') end end diff --git a/spec/lib/banzai/filter/references/label_reference_filter_spec.rb b/spec/lib/banzai/filter/references/label_reference_filter_spec.rb index 55b1a1da07c226..f66af715a99000 100644 --- a/spec/lib/banzai/filter/references/label_reference_filter_spec.rb +++ b/spec/lib/banzai/filter/references/label_reference_filter_spec.rb @@ -277,7 +277,7 @@ end context 'References with html entities' do - let!(:label) { create(:label, title: '<html>', project: project) } + let!(:label) { create(:label, title: '', project: project) } it 'links to a valid reference' do doc = reference_filter('See ~"<html>"') @@ -925,17 +925,15 @@ end it_behaves_like 'a reference which does not unescape its content in data-original' do - let(:context) { { project: project } } - let(:label_title) { "x<script>alert('xss');</script>" } - let(:resource) { create(:label, title: label_title, project: project) } - let(:reference) { %(#{resource.class.reference_prefix}"#{label_title}") } - - # This is probably bad (why are we unescaping entities in the input title like this?), - # but it is the case today, and we need to be safe in spite of this. - let(:expected_resource_title) { "x" } - - let(:expected_href) { urls.project_issues_url(project, label_name: expected_resource_title) } - let(:expected_replacement_text) { expected_resource_title } + let(:context) { { project: project } } + let(:label_title) { "x" } + let(:label_title_escaped) { "x<script>alert('xss');</script>" } + let(:resource) { create(:label, title: label_title, project: project) } + let(:reference) { %(#{resource.class.reference_prefix}"#{label_title_escaped}") } + + let(:expected_resource_title) { label_title } + let(:expected_href) { urls.project_issues_url(project, label_name: label_title) } + let(:expected_replacement_text) { label_title } end it_behaves_like 'ReferenceFilter#references_in' do diff --git a/spec/lib/banzai/filter/references/milestone_reference_filter_spec.rb b/spec/lib/banzai/filter/references/milestone_reference_filter_spec.rb index 7675eacf0cd6f1..822c91d6bd1c6f 100644 --- a/spec/lib/banzai/filter/references/milestone_reference_filter_spec.rb +++ b/spec/lib/banzai/filter/references/milestone_reference_filter_spec.rb @@ -316,7 +316,7 @@ shared_examples 'references with HTML entities' do before do - milestone.update!(title: '<html>') + milestone.update!(title: '') end it 'links to a valid reference' do @@ -629,17 +629,15 @@ end it_behaves_like 'a reference which does not unescape its content in data-original' do - let(:context) { { project: project } } - let(:milestone_title) { "x<script>alert('xss');</script>" } - let(:resource) { create(:milestone, title: milestone_title, project: project) } - let(:reference) { %(#{resource.class.reference_prefix}"#{milestone_title}") } - - # This is probably bad (why are we unescaping entities in the input title like this?), - # but it is the case today, and we need to be safe in spite of this. - let(:expected_resource_title) { "x" } + let(:context) { { project: project } } + let(:milestone_title) { "x" } + let(:milestone_title_escaped) { "x<script>alert('xss');</script>" } + let(:resource) { create(:milestone, title: milestone_title, project: project) } + let(:reference) { %(#{resource.class.reference_prefix}"#{milestone_title_escaped}") } + let(:expected_resource_title) { milestone_title } let(:expected_href) { urls.milestone_url(resource) } - let(:expected_replacement_text) { %(#{resource.class.reference_prefix}#{expected_resource_title}) } + let(:expected_replacement_text) { %(#{resource.class.reference_prefix}#{milestone_title}) } end it_behaves_like 'ReferenceFilter#references_in' do diff --git a/spec/support/shared_examples/lib/banzai/filters/reference_filter_shared_examples.rb b/spec/support/shared_examples/lib/banzai/filters/reference_filter_shared_examples.rb index d6163b19dac9ea..0cb788bfa34db5 100644 --- a/spec/support/shared_examples/lib/banzai/filters/reference_filter_shared_examples.rb +++ b/spec/support/shared_examples/lib/banzai/filters/reference_filter_shared_examples.rb @@ -114,10 +114,11 @@ RSpec.shared_examples 'a reference which does not unescape its content in data-original' do it 'does not remove a layer of escaping in data-original' do # We assert the expected resource.title so calling `it_behaves_like` blocks can better describe - # the known behaviour around the resource. Some classes process their title in ways that are - # particularly relevant to XSS; the big example is Labels, which unescape HTML entities in - # their input. It's helpful to call out when this happens (and when this doesn't) so the rest - # of the behaviours are clear. + # the known behaviour around the resource. + # + # Some classes, like Label and Milestone, used to process their title in ways particularly relevant + # to XSS; they no longer do, but we want to assert they haven't regressed in this manner. + # If they did, these specs would fail in very opaque ways, as their preconditions would no longer hold. expect(resource.title).to eq(expected_resource_title) result = reference_filter("See #{reference}", context) -- GitLab