summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorRobert Speicher <rspeicher@gmail.com>2019-05-28 19:09:33 +0000
committerRobert Speicher <rspeicher@gmail.com>2019-05-28 19:09:33 +0000
commitaffb2b0d11607606414bf5235e199816d517b6b7 (patch)
tree5ec16a6884a6424c39a0635ea5d3105df22c8a57
parent516aeaca2545c80bbb3c336dcfbdd6651695ebe2 (diff)
parentfb59eed035ed4b32720459d267ecacbe4949f3a2 (diff)
downloadgitlab-ce-affb2b0d11607606414bf5235e199816d517b6b7.tar.gz
Merge branch 'security-58856-persistent-xss-11-11' into '11-11-stable'
Persistent XSS in note objects See merge request gitlab/gitlabhq!3127
-rw-r--r--changelogs/unreleased/security-58856-persistent-xss-in-note-objects.yml5
-rw-r--r--lib/gitlab/import_export/attribute_cleaner.rb7
-rw-r--r--spec/features/projects/import_export/export_file_spec.rb2
-rw-r--r--spec/lib/gitlab/import_export/attribute_cleaner_spec.rb6
-rw-r--r--spec/lib/gitlab/import_export/project.json4
-rw-r--r--spec/lib/gitlab/import_export/project_tree_restorer_spec.rb20
6 files changed, 41 insertions, 3 deletions
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/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/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' => '<p>dodgy html</p>',
+ 'legit_html' => '<p>legit html</p>',
+ '_html' => '<p>perfectly ordinary html</p>',
+ '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": "<p>something else entirely</p>",
+ "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": "<p>something else entirely</p>",
+ "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 6084dc96410..9d2b69ea798 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