Commit 2b13462a authored by Robert Speicher's avatar Robert Speicher

Merge branch 'security-58856-persistent-xss-in-note-objects' into 'master'

Persistent XSS in note objects CE

See merge request gitlab/gitlabhq!3075
parents 3dcf3cfd 93a5071a
---
title: Prevent XSS injection in note imports
merge_request:
author:
type: security
......@@ -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)
......
......@@ -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],
......
......@@ -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
......
......@@ -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",
......
......@@ -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
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment