diff --git a/app/models/concerns/base_label.rb b/app/models/concerns/base_label.rb
index b2faa7509e0c48cfb300a1627f2150753d637366..2153655675086bb0fa4837d61c4b7e62b437c37a 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 24150aaae7610605f146988f46236c762514706a..ccef4ad57d53d2002e08f467cab27612cdfce44f 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/lib/banzai/filter/references/iteration_reference_filter_spec.rb b/ee/spec/lib/banzai/filter/references/iteration_reference_filter_spec.rb
index 27c338e245d40c0d6451fbacc916641a2de1274f..0e076953410db17f21116238739ff9c3d70763b0 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 fc1aa60fd3904cb4ebdee3b446d5366b2b1a35a9..82ef964e7bd56c850b5e09b0b2a61879877e1167 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('<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="svgId">')
end
end
diff --git a/ee/spec/models/iteration_spec.rb b/ee/spec/models/iteration_spec.rb
index b6f31284adeb5abd04c7eee15a321064e0ff9bb8..1e74e73429d5a4aa5f9e47611e63390455a6de4c 100644
--- a/ee/spec/models/iteration_spec.rb
+++ b/ee/spec/models/iteration_spec.rb
@@ -313,10 +313,18 @@
end
describe 'title' do
- subject { build(:iteration, iterations_cadence: iteration_cadence, title: '
') }
+ subject { build(:iteration, iterations_cadence: iteration_cadence, title: input_title) }
- it 'sanitizes user intput', :aggregate_failures do
- expect(subject.title).to be_blank
+ 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
diff --git a/spec/controllers/groups/milestones_controller_spec.rb b/spec/controllers/groups/milestones_controller_spec.rb
index 8b34c6e4a43a2459f3b1ce3c0a4f352dc3501c4d..f1f5ff961b6dc761ca1f5641fa8d9a020df1a1a8 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/labels_controller_spec.rb b/spec/controllers/projects/labels_controller_spec.rb
index 40feb6e134877a57f0e1a9cae6e76056267532a2..dc5423528b9f6af3cdad49f2d0201e44e8ecadee 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
diff --git a/spec/controllers/projects/milestones_controller_spec.rb b/spec/controllers/projects/milestones_controller_spec.rb
index 7caa0c0bd57f23bf5e295f33237b336a27da66a2..7a1ac7580f112cca1974109469da3e4628fcf6ca 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
diff --git a/spec/features/issues/issue_sidebar_spec.rb b/spec/features/issues/issue_sidebar_spec.rb
index 9a1a6a9cf2f1a9e8b1958a0862e731cbc9957f87..6c6dc64a99e99026ca71b0dc55d24d6bea9ebcdb 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
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 55b1a1da07c22673f85b4cbfa4033f03673c79c3..f66af715a9900047f57dfb8145eed822aba57eed 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 7675eacf0cd6f1b7b593db9d2a57c389f02ce1ce..822c91d6bd1c6fd3161022c679902772928015b5 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/models/milestone_spec.rb b/spec/models/milestone_spec.rb
index db3fd3d395fc9cd3cef3d257b69f807517de6b0d..bd885e1d2fef09a595080757c9880874c0dac540 100644
--- a/spec/models/milestone_spec.rb
+++ b/spec/models/milestone_spec.rb
@@ -103,13 +103,18 @@
end
describe 'title' do
- it { is_expected.to validate_presence_of(:title) }
+ subject { build(:milestone, project: project, title: input_title) }
- it 'is invalid if title would be empty after sanitation', :aggregate_failures do
- milestone = build(:milestone, project: project, title: '')
+ context 'when input contains HTML entities and HTML tags' do
+ let(:input_title) { '<hello>' }
- expect(milestone).not_to be_valid
- expect(milestone.errors[:title]).to include("can't be blank")
+ 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
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 d6163b19dac9ea426a100ac8185a7c1eeae31a10..0cb788bfa34db51ed4f1a005b2ba7f1e73f6dcdf 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)
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 895fff8cb710022ff1620b8f91bfd1b7081a7063..7a7e54d04eb2349911170e841421d60dc2472ec9 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)
@@ -52,31 +68,25 @@
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?
expect(label.title).to eq('label')
end
+
+ it_behaves_like 'text input field', :title
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?
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 52bedcaa606bc210b1285ac695276556fd21ab3b..9556f3e8b0cdec1171f1de544a72fa73eeb57516 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,23 @@
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')
+ expect(timebox.safe_title).to eq('bfoo-bar-22b')
end
end
- describe "#title" do
- let(:timebox) { create(timebox_type, *timebox_args, title: "foo & bar -> 2.2") }
+ 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 "sanitizes title" do
- expect(timebox.title).to eq("foo & bar -> 2.2")
+ 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