From 0f2dff6264256531d314a8ac7f701f13bcfc7020 Mon Sep 17 00:00:00 2001 From: charlieablett Date: Tue, 23 Apr 2019 20:57:24 +1200 Subject: Exclude fields from note import --- lib/gitlab/import_export/import_export.yml | 3 +++ spec/lib/gitlab/import_export/project.json | 4 ++++ .../import_export/project_tree_restorer_spec.rb | 24 ++++++++++++++++++++-- 3 files changed, 29 insertions(+), 2 deletions(-) diff --git a/lib/gitlab/import_export/import_export.yml b/lib/gitlab/import_export/import_export.yml index ce268793128..ab0c9a32d42 100644 --- a/lib/gitlab/import_export/import_export.yml +++ b/lib/gitlab/import_export/import_export.yml @@ -141,6 +141,9 @@ excluded_attributes: - :external_diff_size issues: - :milestone_id + notes: + - :note_html + - :cached_markdown_version merge_requests: - :milestone_id - :ref_fetched diff --git a/spec/lib/gitlab/import_export/project.json b/spec/lib/gitlab/import_export/project.json index 4a7accc4c52..24d376999fc 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": "

dodgy html

", + "cached_markdown_version": 434343, "noteable_type": "Issue", "author_id": 26, "created_at": "2016-06-14T15:02:47.770Z", @@ -182,6 +184,8 @@ "note": "Est reprehenderit quas aut aspernatur autem recusandae voluptatem.", "noteable_type": "Issue", "author_id": 25, + "note_html": "

dodgy html

", + "cached_markdown_version": 121212, "created_at": "2016-06-14T15:02:47.795Z", "updated_at": "2016-06-14T15:02:47.795Z", "project_id": 5, 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..952844c5d6f 100644 --- a/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb +++ b/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb @@ -7,8 +7,8 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do # Using an admin for import, so we can check assignment of existing members @user = create(:admin) @existing_members = [ - create(:user, username: 'bernard_willms'), - create(:user, username: 'saul_will') + create(:user, username: 'bernard_willms'), + create(:user, username: 'saul_will') ] RSpec::Mocks.with_temporary_scope do @@ -21,6 +21,8 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do expect_any_instance_of(Gitlab::Git::Repository).to receive(:create_branch).with('feature', 'DCBA') allow_any_instance_of(Gitlab::Git::Repository).to receive(:create_branch) + allow_any_instance_of(Project).to receive(:latest_cached_markdown_version).and_return(434343) + allow_any_instance_of(Note).to receive(:latest_cached_markdown_version).and_return(434343) project_tree_restorer = described_class.new(user: @user, shared: @shared, project: @project) @@ -58,6 +60,24 @@ 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 + let!(:issue) { Issue.find_by(description: 'Aliquam enim illo et possimus.') } + let(:note1) { issue.notes.select {|n| n.note.match(/Quo reprehenderit aliquam qui dicta impedit cupiditate eligendi/)}.first } + let(:note2) { issue.notes.select {|n| n.note.match(/Est reprehenderit quas aut aspernatur autem recusandae voluptatem/)}.first } + + it 'does not import the note_html' do + expect(note1.note_html).to match(/Et hic est id similique et non nesciunt voluptate/) + end + + it 'does not set the old cached_markdown_version' do + expect(note2.cached_markdown_version).not_to eq(121212) + end + + it 'does not import the note_html' do + expect(note2.note_html).to match(/Est reprehenderit quas aut aspernatur autem recusandae voluptatem/) + end + end + it 'creates a valid pipeline note' do expect(Ci::Pipeline.find_by_sha('sha-notes').notes).not_to be_empty end -- cgit v1.2.1 From d5bbc33aa58f506614629351a0192d6443c660b5 Mon Sep 17 00:00:00 2001 From: charlieablett Date: Tue, 23 Apr 2019 21:46:26 +1200 Subject: Add changelog entry --- .../unreleased/security-58856-persistent-xss-in-note-objects.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelogs/unreleased/security-58856-persistent-xss-in-note-objects.yml 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 -- cgit v1.2.1 From b8c3e60ea23cd8c6f32ed438fd3c155add47a8e1 Mon Sep 17 00:00:00 2001 From: charlieablett Date: Tue, 23 Apr 2019 23:30:32 +1200 Subject: Re-stub stubbed method calls --- spec/lib/gitlab/import_export/project_tree_restorer_spec.rb | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) 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 952844c5d6f..8853a922e47 100644 --- a/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb +++ b/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb @@ -24,12 +24,12 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do allow_any_instance_of(Project).to receive(:latest_cached_markdown_version).and_return(434343) allow_any_instance_of(Note).to receive(:latest_cached_markdown_version).and_return(434343) - project_tree_restorer = described_class.new(user: @user, shared: @shared, project: @project) + @project_tree_restorer = described_class.new(user: @user, shared: @shared, project: @project) expect(Gitlab::ImportExport::RelationFactory).to receive(:create).with(hash_including(excluded_keys: ['whatever'])).and_call_original.at_least(:once) - allow(project_tree_restorer).to receive(:excluded_keys_for_relation).and_return(['whatever']) + allow(@project_tree_restorer).to receive(:excluded_keys_for_relation).and_return(['whatever']) - @restored_project_json = project_tree_restorer.restore + @restored_project_json = @project_tree_restorer.restore end end @@ -61,6 +61,12 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do end context 'when importing a project with cached_markdown_version and note_html' do + before do + expect(Gitlab::ImportExport::RelationFactory).not_to receive(:create).with(hash_including(excluded_keys: ['whatever'])) + expect(Gitlab::ImportExport::RelationFactory).to receive(:create).with(hash_including(excluded_keys: %w(note_html, cached_markdown_version))).and_call_original.at_least(:once) + allow(@project_tree_restorer).to receive(:excluded_keys_for_relation).and_return(%w(note_html, cached_markdown_version)) + end + let!(:issue) { Issue.find_by(description: 'Aliquam enim illo et possimus.') } let(:note1) { issue.notes.select {|n| n.note.match(/Quo reprehenderit aliquam qui dicta impedit cupiditate eligendi/)}.first } let(:note2) { issue.notes.select {|n| n.note.match(/Est reprehenderit quas aut aspernatur autem recusandae voluptatem/)}.first } -- cgit v1.2.1 From 7e6befc05de87ba44115bbae0274c65727f9c2b9 Mon Sep 17 00:00:00 2001 From: charlieablett Date: Wed, 24 Apr 2019 14:31:20 +1200 Subject: Add disallowed fields to AttributeCleaner --- lib/gitlab/import_export/attribute_cleaner.rb | 14 +++++++++++++- lib/gitlab/import_export/import_export.yml | 3 --- spec/lib/gitlab/import_export/attribute_cleaner_spec.rb | 6 +++++- .../lib/gitlab/import_export/project_tree_restorer_spec.rb | 12 +++--------- 4 files changed, 21 insertions(+), 14 deletions(-) diff --git a/lib/gitlab/import_export/attribute_cleaner.rb b/lib/gitlab/import_export/attribute_cleaner.rb index 93b37b7bc5f..1faa3c1614f 100644 --- a/lib/gitlab/import_export/attribute_cleaner.rb +++ b/lib/gitlab/import_export/attribute_cleaner.rb @@ -24,7 +24,19 @@ module Gitlab private def prohibited_key?(key) - key.end_with?('_id') && !ALLOWED_REFERENCES.include?(key) + return false if allowed_reference?(key) + + return true if 'cached_markdown_version'.equal?(key) + + prohibited_suffices = %w(_id _html) + prohibited_suffices.each do |suffix| + return true if key.end_with?(suffix) + end + false + end + + def allowed_reference?(key) + ALLOWED_REFERENCES.include?(key) end def excluded_key?(key) diff --git a/lib/gitlab/import_export/import_export.yml b/lib/gitlab/import_export/import_export.yml index ab0c9a32d42..ce268793128 100644 --- a/lib/gitlab/import_export/import_export.yml +++ b/lib/gitlab/import_export/import_export.yml @@ -141,9 +141,6 @@ excluded_attributes: - :external_diff_size issues: - :milestone_id - notes: - - :note_html - - :cached_markdown_version merge_requests: - :milestone_id - :ref_fetched 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_tree_restorer_spec.rb b/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb index 8853a922e47..7fd0ea539c2 100644 --- a/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb +++ b/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb @@ -10,6 +10,7 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do create(:user, username: 'bernard_willms'), create(:user, username: 'saul_will') ] + @markdown_classes = [AbuseReport, Appearance, ApplicationSetting, BroadcastMessage, Issue, Label, MergeRequest, Milestone, Namespace, Project, Release, ResourceLabelEvent, Snippet, UserStatus] RSpec::Mocks.with_temporary_scope do @project = create(:project, :builds_enabled, :issues_disabled, name: 'project', path: 'project') @@ -21,8 +22,7 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do expect_any_instance_of(Gitlab::Git::Repository).to receive(:create_branch).with('feature', 'DCBA') allow_any_instance_of(Gitlab::Git::Repository).to receive(:create_branch) - allow_any_instance_of(Project).to receive(:latest_cached_markdown_version).and_return(434343) - allow_any_instance_of(Note).to receive(:latest_cached_markdown_version).and_return(434343) + @markdown_classes.each {|klass| allow_any_instance_of(klass).to receive(:latest_cached_markdown_version).and_return(434343)} @project_tree_restorer = described_class.new(user: @user, shared: @shared, project: @project) @@ -61,18 +61,12 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do end context 'when importing a project with cached_markdown_version and note_html' do - before do - expect(Gitlab::ImportExport::RelationFactory).not_to receive(:create).with(hash_including(excluded_keys: ['whatever'])) - expect(Gitlab::ImportExport::RelationFactory).to receive(:create).with(hash_including(excluded_keys: %w(note_html, cached_markdown_version))).and_call_original.at_least(:once) - allow(@project_tree_restorer).to receive(:excluded_keys_for_relation).and_return(%w(note_html, cached_markdown_version)) - end - let!(:issue) { Issue.find_by(description: 'Aliquam enim illo et possimus.') } let(:note1) { issue.notes.select {|n| n.note.match(/Quo reprehenderit aliquam qui dicta impedit cupiditate eligendi/)}.first } let(:note2) { issue.notes.select {|n| n.note.match(/Est reprehenderit quas aut aspernatur autem recusandae voluptatem/)}.first } it 'does not import the note_html' do - expect(note1.note_html).to match(/Et hic est id similique et non nesciunt voluptate/) + expect(note1.note_html).to match(/Quo reprehenderit aliquam qui dicta impedit cupiditate eligendi/) end it 'does not set the old cached_markdown_version' do -- cgit v1.2.1 From 8eae788fd4248db1fcab2062195e542baf04c936 Mon Sep 17 00:00:00 2001 From: Charlie Ablett Date: Wed, 24 Apr 2019 21:17:53 +0000 Subject: Use English instead of Latin --- lib/gitlab/import_export/attribute_cleaner.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/gitlab/import_export/attribute_cleaner.rb b/lib/gitlab/import_export/attribute_cleaner.rb index 1faa3c1614f..2d64ea77060 100644 --- a/lib/gitlab/import_export/attribute_cleaner.rb +++ b/lib/gitlab/import_export/attribute_cleaner.rb @@ -28,8 +28,8 @@ module Gitlab return true if 'cached_markdown_version'.equal?(key) - prohibited_suffices = %w(_id _html) - prohibited_suffices.each do |suffix| + prohibited_suffixes = %w(_id _html) + prohibited_suffixes.each do |suffix| return true if key.end_with?(suffix) end false -- cgit v1.2.1 From 4bd331a568cff64f4097fdf0cc4e45727750f740 Mon Sep 17 00:00:00 2001 From: charlieablett Date: Fri, 26 Apr 2019 09:40:00 +1200 Subject: Tighten up prohibited_key method --- lib/gitlab/import_export/attribute_cleaner.rb | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/lib/gitlab/import_export/attribute_cleaner.rb b/lib/gitlab/import_export/attribute_cleaner.rb index 2d64ea77060..f476905b6db 100644 --- a/lib/gitlab/import_export/attribute_cleaner.rb +++ b/lib/gitlab/import_export/attribute_cleaner.rb @@ -29,10 +29,9 @@ module Gitlab return true if 'cached_markdown_version'.equal?(key) prohibited_suffixes = %w(_id _html) - prohibited_suffixes.each do |suffix| - return true if key.end_with?(suffix) + prohibited_suffixes.any? do |suffix| + true if key.end_with?(suffix) end - false end def allowed_reference?(key) -- cgit v1.2.1 From b240012c4f474fc9ac24afb3286fbc967cde86d4 Mon Sep 17 00:00:00 2001 From: charlieablett Date: Mon, 29 Apr 2019 21:31:16 +1200 Subject: Further clarify `attribute_cleaner` --- lib/gitlab/import_export/attribute_cleaner.rb | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/lib/gitlab/import_export/attribute_cleaner.rb b/lib/gitlab/import_export/attribute_cleaner.rb index f476905b6db..537cb36c89b 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_SUFFIXES = %w(_id _html).freeze def self.clean(*args) new(*args).clean @@ -17,24 +18,17 @@ module Gitlab def clean @relation_hash.reject do |key, _value| - prohibited_key?(key) || !@relation_class.attribute_method?(key) || excluded_key?(key) + (prohibited_key?(key) && !permitted_key?(key)) || !@relation_class.attribute_method?(key) || excluded_key?(key) end.except('id') end private def prohibited_key?(key) - return false if allowed_reference?(key) - - return true if 'cached_markdown_version'.equal?(key) - - prohibited_suffixes = %w(_id _html) - prohibited_suffixes.any? do |suffix| - true if key.end_with?(suffix) - end + 'cached_markdown_version' == key || PROHIBITED_SUFFIXES.any? {|suffix| key.end_with?(suffix)} end - def allowed_reference?(key) + def permitted_key?(key) ALLOWED_REFERENCES.include?(key) end -- cgit v1.2.1 From 1cbdc5326c779999350481b0aae78364837f3bb6 Mon Sep 17 00:00:00 2001 From: charlieablett Date: Tue, 30 Apr 2019 11:25:09 +1200 Subject: Refactor `attribute_cleaner` for readability --- lib/gitlab/import_export/attribute_cleaner.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/gitlab/import_export/attribute_cleaner.rb b/lib/gitlab/import_export/attribute_cleaner.rb index 537cb36c89b..7f67f63f26b 100644 --- a/lib/gitlab/import_export/attribute_cleaner.rb +++ b/lib/gitlab/import_export/attribute_cleaner.rb @@ -18,13 +18,15 @@ module Gitlab def clean @relation_hash.reject do |key, _value| - (prohibited_key?(key) && !permitted_key?(key)) || !@relation_class.attribute_method?(key) || excluded_key?(key) + prohibited_key?(key) || !@relation_class.attribute_method?(key) || excluded_key?(key) end.except('id') end private def prohibited_key?(key) + return false if permitted_key?(key) + 'cached_markdown_version' == key || PROHIBITED_SUFFIXES.any? {|suffix| key.end_with?(suffix)} end -- cgit v1.2.1 From 3d0fc7fe2e9a5e9f3a24d3eed875044c196e5050 Mon Sep 17 00:00:00 2001 From: Ash McKenzie Date: Tue, 30 Apr 2019 17:43:01 +1200 Subject: Ensure Issue & MR note_html cannot be imported --- spec/lib/gitlab/import_export/project.json | 6 +++-- .../import_export/project_tree_restorer_spec.rb | 26 ++++++++++------------ 2 files changed, 16 insertions(+), 16 deletions(-) diff --git a/spec/lib/gitlab/import_export/project.json b/spec/lib/gitlab/import_export/project.json index 24d376999fc..43afc067e08 100644 --- a/spec/lib/gitlab/import_export/project.json +++ b/spec/lib/gitlab/import_export/project.json @@ -158,8 +158,8 @@ { "id": 351, "note": "Quo reprehenderit aliquam qui dicta impedit cupiditate eligendi.", - "note_html": "

dodgy html

", - "cached_markdown_version": 434343, + "note_html": "

something else entirely

", + "cached_markdown_version": 917504, "noteable_type": "Issue", "author_id": 26, "created_at": "2016-06-14T15:02:47.770Z", @@ -2367,6 +2367,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 7fd0ea539c2..056a1d52d78 100644 --- a/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb +++ b/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb @@ -10,7 +10,6 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do create(:user, username: 'bernard_willms'), create(:user, username: 'saul_will') ] - @markdown_classes = [AbuseReport, Appearance, ApplicationSetting, BroadcastMessage, Issue, Label, MergeRequest, Milestone, Namespace, Project, Release, ResourceLabelEvent, Snippet, UserStatus] RSpec::Mocks.with_temporary_scope do @project = create(:project, :builds_enabled, :issues_disabled, name: 'project', path: 'project') @@ -22,7 +21,6 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do expect_any_instance_of(Gitlab::Git::Repository).to receive(:create_branch).with('feature', 'DCBA') allow_any_instance_of(Gitlab::Git::Repository).to receive(:create_branch) - @markdown_classes.each {|klass| allow_any_instance_of(klass).to receive(:latest_cached_markdown_version).and_return(434343)} @project_tree_restorer = described_class.new(user: @user, shared: @shared, project: @project) @@ -61,20 +59,20 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do end context 'when importing a project with cached_markdown_version and note_html' do - let!(:issue) { Issue.find_by(description: 'Aliquam enim illo et possimus.') } - let(:note1) { issue.notes.select {|n| n.note.match(/Quo reprehenderit aliquam qui dicta impedit cupiditate eligendi/)}.first } - let(:note2) { issue.notes.select {|n| n.note.match(/Est reprehenderit quas aut aspernatur autem recusandae voluptatem/)}.first } - - it 'does not import the note_html' do - expect(note1.note_html).to match(/Quo reprehenderit aliquam qui dicta impedit cupiditate eligendi/) - end - - it 'does not set the old cached_markdown_version' do - expect(note2.cached_markdown_version).not_to eq(121212) + 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 - it 'does not import the note_html' do - expect(note2.note_html).to match(/Est reprehenderit quas aut aspernatur autem recusandae voluptatem/) + 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 -- cgit v1.2.1 From f2bc55d76f278e492902bec99534600d589177b7 Mon Sep 17 00:00:00 2001 From: charlieablett Date: Tue, 30 Apr 2019 20:57:52 +1200 Subject: Remove accidental regressions --- spec/lib/gitlab/import_export/project_tree_restorer_spec.rb | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) 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 056a1d52d78..9aafa41feee 100644 --- a/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb +++ b/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb @@ -7,8 +7,8 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do # Using an admin for import, so we can check assignment of existing members @user = create(:admin) @existing_members = [ - create(:user, username: 'bernard_willms'), - create(:user, username: 'saul_will') + create(:user, username: 'bernard_willms'), + create(:user, username: 'saul_will') ] RSpec::Mocks.with_temporary_scope do @@ -22,12 +22,12 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do expect_any_instance_of(Gitlab::Git::Repository).to receive(:create_branch).with('feature', 'DCBA') allow_any_instance_of(Gitlab::Git::Repository).to receive(:create_branch) - @project_tree_restorer = described_class.new(user: @user, shared: @shared, project: @project) + project_tree_restorer = described_class.new(user: @user, shared: @shared, project: @project) expect(Gitlab::ImportExport::RelationFactory).to receive(:create).with(hash_including(excluded_keys: ['whatever'])).and_call_original.at_least(:once) - allow(@project_tree_restorer).to receive(:excluded_keys_for_relation).and_return(['whatever']) + allow(project_tree_restorer).to receive(:excluded_keys_for_relation).and_return(['whatever']) - @restored_project_json = @project_tree_restorer.restore + @restored_project_json = project_tree_restorer.restore end end @@ -76,6 +76,7 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do end end + it 'creates a valid pipeline note' do expect(Ci::Pipeline.find_by_sha('sha-notes').notes).not_to be_empty end -- cgit v1.2.1 From 4b46b530829cc3dd82c2620a76fbe637ca9009c0 Mon Sep 17 00:00:00 2001 From: charlieablett Date: Wed, 1 May 2019 10:38:41 +1200 Subject: Add `html` to sensitive words --- lib/gitlab/import_export/attribute_cleaner.rb | 2 +- spec/features/projects/import_export/export_file_spec.rb | 2 +- spec/lib/gitlab/import_export/project_tree_restorer_spec.rb | 3 ++- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/gitlab/import_export/attribute_cleaner.rb b/lib/gitlab/import_export/attribute_cleaner.rb index 7f67f63f26b..7bdef2b6cdb 100644 --- a/lib/gitlab/import_export/attribute_cleaner.rb +++ b/lib/gitlab/import_export/attribute_cleaner.rb @@ -4,7 +4,7 @@ module Gitlab module ImportExport class AttributeCleaner ALLOWED_REFERENCES = RelationFactory::PROJECT_REFERENCES + RelationFactory::USER_REFERENCES + ['group_id'] - PROHIBITED_SUFFIXES = %w(_id _html).freeze + PROHIBITED_SUFFIXES = %w[_id _html].freeze def self.clean(*args) new(*args).clean 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/project_tree_restorer_spec.rb b/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb index 9aafa41feee..9d2b69ea798 100644 --- a/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb +++ b/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb @@ -63,6 +63,7 @@ describe Gitlab::ImportExport::ProjectTreeRestorer 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 @@ -71,12 +72,12 @@ describe Gitlab::ImportExport::ProjectTreeRestorer 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 -- cgit v1.2.1 From 0aff6238f777155750c567ab409d0bce60536526 Mon Sep 17 00:00:00 2001 From: charlieablett Date: Wed, 1 May 2019 12:15:29 +1200 Subject: Change `prohibited_key` to use regexes --- lib/gitlab/import_export/attribute_cleaner.rb | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/lib/gitlab/import_export/attribute_cleaner.rb b/lib/gitlab/import_export/attribute_cleaner.rb index 7bdef2b6cdb..c28a1674018 100644 --- a/lib/gitlab/import_export/attribute_cleaner.rb +++ b/lib/gitlab/import_export/attribute_cleaner.rb @@ -4,7 +4,7 @@ module Gitlab module ImportExport class AttributeCleaner ALLOWED_REFERENCES = RelationFactory::PROJECT_REFERENCES + RelationFactory::USER_REFERENCES + ['group_id'] - PROHIBITED_SUFFIXES = %w[_id _html].freeze + PROHIBITED_REFERENCES = Regexp.union(/\Acached_markdown_version\Z/, /_id\Z/, /_html\Z/).freeze def self.clean(*args) new(*args).clean @@ -25,9 +25,7 @@ module Gitlab private def prohibited_key?(key) - return false if permitted_key?(key) - - 'cached_markdown_version' == key || PROHIBITED_SUFFIXES.any? {|suffix| key.end_with?(suffix)} + key =~ PROHIBITED_REFERENCES && !permitted_key?(key) end def permitted_key?(key) -- cgit v1.2.1 From a76fdcb7a30c6244ffb11a2e672e16d1e5b413b2 Mon Sep 17 00:00:00 2001 From: Kerri Miller Date: Mon, 20 May 2019 13:24:22 -0700 Subject: Reject slug+uri concat if slug is deemed unsafe First reported: https://gitlab.com/gitlab-org/gitlab-ce/issues/60143 When the page slug is "javascript:" and we attempt to link to a relative path (using `.` or `..`) the code will concatenate the slug and the uri. This MR adds a guard to that concat step that will return `nil` if the incoming slug matches against any of the "unsafe" slug regexes; currently this is only for the slug "javascript:" but can be extended if needed. Manually tested against a non-exhaustive list from OWASP of common javascript XSS exploits that have to to with mangling the "javascript:" method, and all are caught by this change or by existing code that ingests the user-specified slug. --- ...urity-60143-address-xss-issue-in-wiki-links.yml | 5 +++ lib/banzai/filter/wiki_link_filter/rewriter.rb | 8 +++++ spec/lib/banzai/filter/wiki_link_filter_spec.rb | 42 ++++++++++++++++++++++ 3 files changed, 55 insertions(+) create mode 100644 changelogs/unreleased/security-60143-address-xss-issue-in-wiki-links.yml 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/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 -- cgit v1.2.1 From 93a5071a3bde5db57f8f763a1c958a00fa6b4bf8 Mon Sep 17 00:00:00 2001 From: Tiger Date: Tue, 28 May 2019 09:44:45 -0500 Subject: Remove unused fixture lines --- spec/lib/gitlab/import_export/project.json | 2 -- 1 file changed, 2 deletions(-) diff --git a/spec/lib/gitlab/import_export/project.json b/spec/lib/gitlab/import_export/project.json index 43afc067e08..fb7bddb386c 100644 --- a/spec/lib/gitlab/import_export/project.json +++ b/spec/lib/gitlab/import_export/project.json @@ -184,8 +184,6 @@ "note": "Est reprehenderit quas aut aspernatur autem recusandae voluptatem.", "noteable_type": "Issue", "author_id": 25, - "note_html": "

dodgy html

", - "cached_markdown_version": 121212, "created_at": "2016-06-14T15:02:47.795Z", "updated_at": "2016-06-14T15:02:47.795Z", "project_id": 5, -- cgit v1.2.1