diff --git a/changelogs/unreleased/security-58856-persistent-xss-in-note-objects.yml b/changelogs/unreleased/security-58856-persistent-xss-in-note-objects.yml
new file mode 100644
index 00000000000..d9ad5af256a
--- /dev/null
+++ b/changelogs/unreleased/security-58856-persistent-xss-in-note-objects.yml
@@ -0,0 +1,5 @@
+---
+title: Prevent XSS injection in note imports
+merge_request:
+author:
+type: security
diff --git a/changelogs/unreleased/security-60143-address-xss-issue-in-wiki-links.yml b/changelogs/unreleased/security-60143-address-xss-issue-in-wiki-links.yml
new file mode 100644
index 00000000000..5b79258af54
--- /dev/null
+++ b/changelogs/unreleased/security-60143-address-xss-issue-in-wiki-links.yml
@@ -0,0 +1,5 @@
+---
+title: Filter relative links in wiki for XSS
+merge_request:
+author:
+type: security
diff --git a/lib/banzai/filter/wiki_link_filter/rewriter.rb b/lib/banzai/filter/wiki_link_filter/rewriter.rb
index f4cc8beeb52..77b5053f38c 100644
--- a/lib/banzai/filter/wiki_link_filter/rewriter.rb
+++ b/lib/banzai/filter/wiki_link_filter/rewriter.rb
@@ -4,6 +4,8 @@ module Banzai
module Filter
class WikiLinkFilter < HTML::Pipeline::Filter
class Rewriter
+ UNSAFE_SLUG_REGEXES = [/\Ajavascript:/i].freeze
+
def initialize(link_string, wiki:, slug:)
@uri = Addressable::URI.parse(link_string)
@wiki_base_path = wiki && wiki.wiki_base_path
@@ -35,6 +37,8 @@ module Banzai
# Of the form `./link`, `../link`, or similar
def apply_hierarchical_link_rules!
+ return if slug_considered_unsafe?
+
@uri = Addressable::URI.join(@slug, @uri) if @uri.to_s[0] == '.'
end
@@ -54,6 +58,10 @@ module Banzai
def repository_upload?
@uri.relative? && @uri.path.starts_with?(Wikis::CreateAttachmentService::ATTACHMENT_PATH)
end
+
+ def slug_considered_unsafe?
+ UNSAFE_SLUG_REGEXES.any? { |r| r.match?(@slug) }
+ end
end
end
end
diff --git a/lib/gitlab/import_export/attribute_cleaner.rb b/lib/gitlab/import_export/attribute_cleaner.rb
index 93b37b7bc5f..c28a1674018 100644
--- a/lib/gitlab/import_export/attribute_cleaner.rb
+++ b/lib/gitlab/import_export/attribute_cleaner.rb
@@ -4,6 +4,7 @@ module Gitlab
module ImportExport
class AttributeCleaner
ALLOWED_REFERENCES = RelationFactory::PROJECT_REFERENCES + RelationFactory::USER_REFERENCES + ['group_id']
+ PROHIBITED_REFERENCES = Regexp.union(/\Acached_markdown_version\Z/, /_id\Z/, /_html\Z/).freeze
def self.clean(*args)
new(*args).clean
@@ -24,7 +25,11 @@ module Gitlab
private
def prohibited_key?(key)
- key.end_with?('_id') && !ALLOWED_REFERENCES.include?(key)
+ key =~ PROHIBITED_REFERENCES && !permitted_key?(key)
+ end
+
+ def permitted_key?(key)
+ ALLOWED_REFERENCES.include?(key)
end
def excluded_key?(key)
diff --git a/spec/features/projects/import_export/export_file_spec.rb b/spec/features/projects/import_export/export_file_spec.rb
index f76f9ba7577..9d74a96ab3d 100644
--- a/spec/features/projects/import_export/export_file_spec.rb
+++ b/spec/features/projects/import_export/export_file_spec.rb
@@ -12,7 +12,7 @@ describe 'Import/Export - project export integration test', :js do
let(:export_path) { "#{Dir.tmpdir}/import_file_spec" }
let(:config_hash) { YAML.load_file(Gitlab::ImportExport.config_file).deep_stringify_keys }
- let(:sensitive_words) { %w[pass secret token key encrypted] }
+ let(:sensitive_words) { %w[pass secret token key encrypted html] }
let(:safe_list) do
{
token: [ProjectHook, Ci::Trigger, CommitStatus],
diff --git a/spec/lib/banzai/filter/wiki_link_filter_spec.rb b/spec/lib/banzai/filter/wiki_link_filter_spec.rb
index b9059b85fdc..cce1cd0b284 100644
--- a/spec/lib/banzai/filter/wiki_link_filter_spec.rb
+++ b/spec/lib/banzai/filter/wiki_link_filter_spec.rb
@@ -70,5 +70,47 @@ describe Banzai::Filter::WikiLinkFilter do
expect(filtered_link.attribute('href').value).to eq(invalid_link)
end
end
+
+ context "when the slug is deemed unsafe or invalid" do
+ let(:link) { "alert(1);" }
+
+ invalid_slugs = [
+ "javascript:",
+ "JaVaScRiPt:",
+ "\u0001java\u0003script:",
+ "javascript :",
+ "javascript: ",
+ "javascript : ",
+ ":javascript:",
+ "javascript:",
+ "javascript:",
+ "javascript:",
+ "javascript:",
+ "java\0script:",
+ " javascript:"
+ ]
+
+ invalid_slugs.each do |slug|
+ context "with the slug #{slug}" do
+ it "doesn't rewrite a (.) relative link" do
+ filtered_link = filter(
+ "Link",
+ project_wiki: wiki,
+ page_slug: slug).children[0]
+
+ expect(filtered_link.attribute('href').value).not_to include(slug)
+ end
+
+ it "doesn't rewrite a (..) relative link" do
+ filtered_link = filter(
+ "Link",
+ project_wiki: wiki,
+ page_slug: slug).children[0]
+
+ expect(filtered_link.attribute('href').value).not_to include(slug)
+ end
+ end
+ end
+ end
end
end
diff --git a/spec/lib/gitlab/import_export/attribute_cleaner_spec.rb b/spec/lib/gitlab/import_export/attribute_cleaner_spec.rb
index 536cc359d39..99669285d5b 100644
--- a/spec/lib/gitlab/import_export/attribute_cleaner_spec.rb
+++ b/spec/lib/gitlab/import_export/attribute_cleaner_spec.rb
@@ -18,7 +18,11 @@ describe Gitlab::ImportExport::AttributeCleaner do
'notid' => 99,
'import_source' => 'whatever',
'import_type' => 'whatever',
- 'non_existent_attr' => 'whatever'
+ 'non_existent_attr' => 'whatever',
+ 'some_html' => 'dodgy html
',
+ 'legit_html' => 'legit html
',
+ '_html' => 'perfectly ordinary html
',
+ 'cached_markdown_version' => 12345
}
end
diff --git a/spec/lib/gitlab/import_export/project.json b/spec/lib/gitlab/import_export/project.json
index 4a7accc4c52..fb7bddb386c 100644
--- a/spec/lib/gitlab/import_export/project.json
+++ b/spec/lib/gitlab/import_export/project.json
@@ -158,6 +158,8 @@
{
"id": 351,
"note": "Quo reprehenderit aliquam qui dicta impedit cupiditate eligendi.",
+ "note_html": "something else entirely
",
+ "cached_markdown_version": 917504,
"noteable_type": "Issue",
"author_id": 26,
"created_at": "2016-06-14T15:02:47.770Z",
@@ -2363,6 +2365,8 @@
{
"id": 671,
"note": "Sit voluptatibus eveniet architecto quidem.",
+ "note_html": "something else entirely
",
+ "cached_markdown_version": 917504,
"noteable_type": "MergeRequest",
"author_id": 26,
"created_at": "2016-06-14T15:02:56.632Z",
diff --git a/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb b/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb
index 651aa600fb2..ca46006ea58 100644
--- a/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb
+++ b/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb
@@ -58,6 +58,26 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do
expect(Milestone.find_by_description('test milestone').issues.count).to eq(2)
end
+ context 'when importing a project with cached_markdown_version and note_html' do
+ context 'for an Issue' do
+ it 'does not import note_html' do
+ note_content = 'Quo reprehenderit aliquam qui dicta impedit cupiditate eligendi'
+ issue_note = Issue.find_by(description: 'Aliquam enim illo et possimus.').notes.select { |n| n.note.match(/#{note_content}/)}.first
+
+ expect(issue_note.note_html).to match(/#{note_content}/)
+ end
+ end
+
+ context 'for a Merge Request' do
+ it 'does not import note_html' do
+ note_content = 'Sit voluptatibus eveniet architecto quidem'
+ merge_request_note = MergeRequest.find_by(title: 'MR1').notes.select { |n| n.note.match(/#{note_content}/)}.first
+
+ expect(merge_request_note.note_html).to match(/#{note_content}/)
+ end
+ end
+ end
+
it 'creates a valid pipeline note' do
expect(Ci::Pipeline.find_by_sha('sha-notes').notes).not_to be_empty
end