Merge branch 'master' of dev.gitlab.org:gitlab/gitlabhq

changelogs/unreleased/security-58856-persistent-xss-in-note-objects.yml

+---
+title: Prevent XSS injection in note imports
+merge_request:
+author:
+type: security

changelogs/unreleased/security-60143-address-xss-issue-in-wiki-links.yml

+---
+title: Filter relative links in wiki for XSS
+merge_request:
+author:
+type: security

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

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)

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],

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(
+              "<a href='.#{link}'>Link</a>",
+              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(
+              "<a href='..#{link}'>Link</a>",
+              project_wiki: wiki,
+              page_slug: slug).children[0]
+
+            expect(filtered_link.attribute('href').value).not_to include(slug)
+          end
+        end
+      end
+    end
   end
 end

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
 

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",

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