Add latest changes from gitlab-org/security/gitlab@15-3-stable-ee

15 files changed, 156 insertions, 48 deletions

diff --git a/app/graphql/resolvers/paginated_tree_resolver.rb b/app/graphql/resolvers/paginated_tree_resolver.rb
index 1b4211366e0..c7e9e522c25 100644
--- a/app/graphql/resolvers/paginated_tree_resolver.rb
+++ b/app/graphql/resolvers/paginated_tree_resolver.rb
@@ -32,7 +32,11 @@ module Resolvers
         page_token: cursor
       }
 
-      tree = repository.tree(args[:ref], args[:path], recursive: args[:recursive], pagination_params: pagination_params)
+      tree = repository.tree(
+        args[:ref], args[:path], recursive: args[:recursive],
+                                 skip_flat_paths: false,
+                                 pagination_params: pagination_params
+      )
 
       next_cursor = tree.cursor&.next_cursor
       Gitlab::Graphql::ExternallyPaginatedArray.new(cursor, next_cursor, *tree)
diff --git a/app/models/repository.rb b/app/models/repository.rb
index eb8e45877f3..26c3b01a46e 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -677,24 +677,24 @@ class Repository
     @head_commit ||= commit(self.root_ref)
   end
 
-  def head_tree
+  def head_tree(skip_flat_paths: true)
     if head_commit
-      @head_tree ||= Tree.new(self, head_commit.sha, nil)
+      @head_tree ||= Tree.new(self, head_commit.sha, nil, skip_flat_paths: skip_flat_paths)
     end
   end
 
-  def tree(sha = :head, path = nil, recursive: false, pagination_params: nil)
+  def tree(sha = :head, path = nil, recursive: false, skip_flat_paths: true, pagination_params: nil)
     if sha == :head
       return unless head_commit
 
       if path.nil?
-        return head_tree
+        return head_tree(skip_flat_paths: skip_flat_paths)
       else
         sha = head_commit.sha
       end
     end
 
-    Tree.new(self, sha, path, recursive: recursive, pagination_params: pagination_params)
+    Tree.new(self, sha, path, recursive: recursive, skip_flat_paths: skip_flat_paths, pagination_params: pagination_params)
   end
 
   def blob_at_branch(branch_name, path)
diff --git a/app/models/tree.rb b/app/models/tree.rb
index fd416ebdedc..941d0394b94 100644
--- a/app/models/tree.rb
+++ b/app/models/tree.rb
@@ -6,7 +6,7 @@ class Tree
 
   attr_accessor :repository, :sha, :path, :entries, :cursor
 
-  def initialize(repository, sha, path = '/', recursive: false, pagination_params: nil)
+  def initialize(repository, sha, path = '/', recursive: false, skip_flat_paths: true, pagination_params: nil)
     path = '/' if path.blank?
 
     @repository = repository
@@ -14,7 +14,7 @@ class Tree
     @path = path
 
     git_repo = @repository.raw_repository
-    @entries, @cursor = Gitlab::Git::Tree.where(git_repo, @sha, @path, recursive, pagination_params)
+    @entries, @cursor = Gitlab::Git::Tree.where(git_repo, @sha, @path, recursive, skip_flat_paths, pagination_params)
   end
 
   def readme_path
diff --git a/lib/banzai/filter/commit_trailers_filter.rb b/lib/banzai/filter/commit_trailers_filter.rb
index a615abc1989..817bea42757 100644
--- a/lib/banzai/filter/commit_trailers_filter.rb
+++ b/lib/banzai/filter/commit_trailers_filter.rb
@@ -17,21 +17,10 @@ module Banzai
       include ActionView::Helpers::TagHelper
       include AvatarsHelper
 
-      TRAILER_REGEXP = /(?<label>[[:alpha:]-]+-by:)/i.freeze
-      AUTHOR_REGEXP = /(?<author_name>.+)/.freeze
-      # Devise.email_regexp wouldn't work here since its designed to match
-      # against strings that only contains email addresses; the \A and \z
-      # around the expression will only match if the string being matched
-      # contains just the email nothing else.
-      MAIL_REGEXP = /&lt;(?<author_email>[^@\s]+@[^@\s]+)&gt;/.freeze
-      FILTER_REGEXP = /(?<trailer>^\s*#{TRAILER_REGEXP}\s*#{AUTHOR_REGEXP}\s+#{MAIL_REGEXP}$)/mi.freeze
-
       def call
         doc.xpath('descendant-or-self::text()').each do |node|
           content = node.to_html
 
-          next unless content.match(FILTER_REGEXP)
-
           html = trailer_filter(content)
 
           next if html == content
@@ -52,11 +41,24 @@ module Banzai
       # Returns a String with all trailer lines replaced with links to GitLab
       # users and mailto links to non GitLab users. All links have `data-trailer`
       # and `data-user` attributes attached.
+      #
+      # The code intentionally avoids using Regex for security and performance
+      # reasons: https://gitlab.com/gitlab-org/gitlab/-/issues/363734
       def trailer_filter(text)
-        text.gsub(FILTER_REGEXP) do |author_match|
-          label = $~[:label]
-          "#{label} #{parse_user($~[:author_name], $~[:author_email], label)}"
-        end
+        text.lines.map! do |line|
+          trailer, rest = line.split(':', 2)
+
+          next line unless trailer.downcase.end_with?('-by') && rest.present?
+
+          chunks = rest.split
+          author_email = chunks.pop.delete_prefix('&lt;').delete_suffix('&gt;')
+          next line unless Devise.email_regexp.match(author_email)
+
+          author_name = chunks.join(' ').strip
+          trailer = "#{trailer.strip}:"
+
+          "#{trailer} #{link_to_user_or_email(author_name, author_email, trailer)}\n"
+        end.join
       end
 
       # Find a GitLab user using the supplied email and generate
@@ -67,7 +69,7 @@ module Banzai
       # trailer - String trailer used in the commit message
       #
       # Returns a String with a link to the user.
-      def parse_user(name, email, trailer)
+      def link_to_user_or_email(name, email, trailer)
         link_to_user User.find_by_any_email(email),
           name: name,
           email: email,
diff --git a/lib/banzai/filter/pathological_markdown_filter.rb b/lib/banzai/filter/pathological_markdown_filter.rb
new file mode 100644
index 00000000000..0f94150c7a1
--- /dev/null
+++ b/lib/banzai/filter/pathological_markdown_filter.rb
@@ -0,0 +1,27 @@
+# frozen_string_literal: true
+
+module Banzai
+  module Filter
+    class PathologicalMarkdownFilter < HTML::Pipeline::TextFilter
+      # It's not necessary for this to be precise - we just need to detect
+      # when there are a non-trivial number of unclosed image links.
+      # So we don't really care about code blocks, etc.
+      # See https://gitlab.com/gitlab-org/gitlab/-/issues/370428
+      REGEX = /!\[(?:[^\]])+?!\[/.freeze
+      DETECTION_MAX = 10
+
+      def call
+        count = 0
+
+        @text.scan(REGEX) do |_match|
+          count += 1
+          break if count > DETECTION_MAX
+        end
+
+        return @text if count <= DETECTION_MAX
+
+        "_Unable to render markdown - too many unclosed markdown image links detected._"
+      end
+    end
+  end
+end
diff --git a/lib/banzai/pipeline/plain_markdown_pipeline.rb b/lib/banzai/pipeline/plain_markdown_pipeline.rb
index 1da0f72996b..fb6f6e9077d 100644
--- a/lib/banzai/pipeline/plain_markdown_pipeline.rb
+++ b/lib/banzai/pipeline/plain_markdown_pipeline.rb
@@ -5,6 +5,7 @@ module Banzai
     class PlainMarkdownPipeline < BasePipeline
       def self.filters
         FilterArray[
+          Filter::PathologicalMarkdownFilter,
           Filter::MarkdownPreEscapeFilter,
           Filter::MarkdownFilter,
           Filter::MarkdownPostEscapeFilter
diff --git a/lib/gitlab/git/rugged_impl/tree.rb b/lib/gitlab/git/rugged_impl/tree.rb
index bc0af12d7e3..66cfc02130b 100644
--- a/lib/gitlab/git/rugged_impl/tree.rb
+++ b/lib/gitlab/git/rugged_impl/tree.rb
@@ -16,9 +16,10 @@ module Gitlab
           TREE_SORT_ORDER = { tree: 0, blob: 1, commit: 2 }.freeze
 
           override :tree_entries
-          def tree_entries(repository, sha, path, recursive, pagination_params = nil)
+          def tree_entries(repository, sha, path, recursive, skip_flat_paths, pagination_params = nil)
             if use_rugged?(repository, :rugged_tree_entries)
-              entries = execute_rugged_call(:tree_entries_with_flat_path_from_rugged, repository, sha, path, recursive)
+              entries = execute_rugged_call(
+                :tree_entries_with_flat_path_from_rugged, repository, sha, path, recursive, skip_flat_paths)
 
               if pagination_params
                 paginated_response(entries, pagination_params[:limit], pagination_params[:page_token].to_s)
@@ -60,11 +61,11 @@ module Gitlab
             [result, cursor]
           end
 
-          def tree_entries_with_flat_path_from_rugged(repository, sha, path, recursive)
+          def tree_entries_with_flat_path_from_rugged(repository, sha, path, recursive, skip_flat_paths)
             tree_entries_from_rugged(repository, sha, path, recursive).tap do |entries|
               # This was an optimization to reduce N+1 queries for Gitaly
               # (https://gitlab.com/gitlab-org/gitaly/issues/530).
-              rugged_populate_flat_path(repository, sha, path, entries)
+              rugged_populate_flat_path(repository, sha, path, entries) unless skip_flat_paths
             end
           end
 
diff --git a/lib/gitlab/git/tree.rb b/lib/gitlab/git/tree.rb
index eb008507397..f0eef619e13 100644
--- a/lib/gitlab/git/tree.rb
+++ b/lib/gitlab/git/tree.rb
@@ -15,15 +15,16 @@ module Gitlab
         # Uses rugged for raw objects
         #
         # Gitaly migration: https://gitlab.com/gitlab-org/gitaly/issues/320
-        def where(repository, sha, path = nil, recursive = false, pagination_params = nil)
+        def where(repository, sha, path = nil, recursive = false, skip_flat_paths = true, pagination_params = nil)
           path = nil if path == '' || path == '/'
 
-          tree_entries(repository, sha, path, recursive, pagination_params)
+          tree_entries(repository, sha, path, recursive, skip_flat_paths, pagination_params)
         end
 
-        def tree_entries(repository, sha, path, recursive, pagination_params = nil)
+        def tree_entries(repository, sha, path, recursive, skip_flat_paths, pagination_params = nil)
           wrapped_gitaly_errors do
-            repository.gitaly_commit_client.tree_entries(repository, sha, path, recursive, pagination_params)
+            repository.gitaly_commit_client.tree_entries(
+              repository, sha, path, recursive, skip_flat_paths, pagination_params)
           end
         end
 
diff --git a/lib/gitlab/gitaly_client/commit_service.rb b/lib/gitlab/gitaly_client/commit_service.rb
index 9fb34f74c82..0f306a9825d 100644
--- a/lib/gitlab/gitaly_client/commit_service.rb
+++ b/lib/gitlab/gitaly_client/commit_service.rb
@@ -5,6 +5,8 @@ module Gitlab
     class CommitService
       include Gitlab::EncodingHelper
 
+      TREE_ENTRIES_DEFAULT_LIMIT = 100_000
+
       def initialize(repository)
         @gitaly_repo = repository.gitaly_repository
         @repository = repository
@@ -111,12 +113,16 @@ module Gitlab
         nil
       end
 
-      def tree_entries(repository, revision, path, recursive, pagination_params)
+      def tree_entries(repository, revision, path, recursive, skip_flat_paths, pagination_params)
+        pagination_params ||= {}
+        pagination_params[:limit] ||= TREE_ENTRIES_DEFAULT_LIMIT
+
         request = Gitaly::GetTreeEntriesRequest.new(
           repository: @gitaly_repo,
           revision: encode_binary(revision),
           path: path.present? ? encode_binary(path) : '.',
           recursive: recursive,
+          skip_flat_paths: skip_flat_paths,
           pagination_params: pagination_params
         )
         request.sort = Gitaly::GetTreeEntriesRequest::SortBy::TREES_FIRST if pagination_params
diff --git a/spec/lib/banzai/filter/commit_trailers_filter_spec.rb b/spec/lib/banzai/filter/commit_trailers_filter_spec.rb
index 38f9bda57e6..c22517621c1 100644
--- a/spec/lib/banzai/filter/commit_trailers_filter_spec.rb
+++ b/spec/lib/banzai/filter/commit_trailers_filter_spec.rb
@@ -18,10 +18,20 @@ RSpec.describe Banzai::Filter::CommitTrailersFilter do
   context 'detects' do
     let(:email) { FFaker::Internet.email }
 
-    it 'trailers in the form of *-by and replace users with links' do
-      doc = filter(commit_message_html)
+    context 'trailers in the form of *-by' do
+      where(:commit_trailer) do
+        ["#{FFaker::Lorem.word}-by:", "#{FFaker::Lorem.word}-BY:", "#{FFaker::Lorem.word}-By:"]
+      end
 
-      expect_to_have_user_link_with_avatar(doc, user: user, trailer: trailer)
+      with_them do
+        let(:trailer) { commit_trailer }
+
+        it 'replaces users with links' do
+          doc = filter(commit_message_html)
+
+          expect_to_have_user_link_with_avatar(doc, user: user, trailer: trailer)
+        end
+      end
     end
 
     it 'trailers prefixed with whitespaces' do
@@ -121,7 +131,14 @@ RSpec.describe Banzai::Filter::CommitTrailersFilter do
 
   context "ignores" do
     it 'commit messages without trailers' do
-      exp = message = commit_html(FFaker::Lorem.sentence)
+      exp = message = commit_html(Array.new(5) { FFaker::Lorem.sentence }.join("\n"))
+      doc = filter(message)
+
+      expect(doc.to_html).to match Regexp.escape(exp)
+    end
+
+    it 'trailers without emails' do
+      exp = message = commit_html(Array.new(5) { 'Merged-By:' }.join("\n"))
       doc = filter(message)
 
       expect(doc.to_html).to match Regexp.escape(exp)
diff --git a/spec/lib/banzai/filter/pathological_markdown_filter_spec.rb b/spec/lib/banzai/filter/pathological_markdown_filter_spec.rb
new file mode 100644
index 00000000000..e0a07d1ea77
--- /dev/null
+++ b/spec/lib/banzai/filter/pathological_markdown_filter_spec.rb
@@ -0,0 +1,27 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Banzai::Filter::PathologicalMarkdownFilter do
+  include FilterSpecHelper
+
+  let_it_be(:short_text) { '![a' * 5 }
+  let_it_be(:long_text) { ([short_text] * 10).join(' ') }
+  let_it_be(:with_images_text) { "![One ![one](one.jpg) #{'and\n' * 200} ![two ![two](two.jpg)" }
+
+  it 'detects a significat number of unclosed image links' do
+    msg = <<~TEXT
+      _Unable to render markdown - too many unclosed markdown image links detected._
+    TEXT
+
+    expect(filter(long_text)).to eq(msg.strip)
+  end
+
+  it 'does nothing when there are only a few unclosed image links' do
+    expect(filter(short_text)).to eq(short_text)
+  end
+
+  it 'does nothing when there are only a few unclosed image links and images' do
+    expect(filter(with_images_text)).to eq(with_images_text)
+  end
+end
diff --git a/spec/lib/banzai/pipeline/full_pipeline_spec.rb b/spec/lib/banzai/pipeline/full_pipeline_spec.rb
index 376edfb99fc..c07f99dc9fc 100644
--- a/spec/lib/banzai/pipeline/full_pipeline_spec.rb
+++ b/spec/lib/banzai/pipeline/full_pipeline_spec.rb
@@ -167,4 +167,16 @@ RSpec.describe Banzai::Pipeline::FullPipeline do
       expect(output).to include('<em>@test_</em>')
     end
   end
+
+  describe 'unclosed image links' do
+    it 'detects a significat number of unclosed image links' do
+      markdown = '![a ' * 30
+      msg = <<~TEXT
+        Unable to render markdown - too many unclosed markdown image links detected.
+      TEXT
+      output = described_class.to_html(markdown, project: nil)
+
+      expect(output).to include(msg.strip)
+    end
+  end
 end
diff --git a/spec/lib/gitlab/git/tree_spec.rb b/spec/lib/gitlab/git/tree_spec.rb
index b520de03929..2e4520cd3a0 100644
--- a/spec/lib/gitlab/git/tree_spec.rb
+++ b/spec/lib/gitlab/git/tree_spec.rb
@@ -9,12 +9,13 @@ RSpec.describe Gitlab::Git::Tree do
   let(:repository) { project.repository.raw }
 
   shared_examples :repo do
-    subject(:tree) { Gitlab::Git::Tree.where(repository, sha, path, recursive, pagination_params) }
+    subject(:tree) { Gitlab::Git::Tree.where(repository, sha, path, recursive, skip_flat_paths, pagination_params) }
 
     let(:sha) { SeedRepo::Commit::ID }
     let(:path) { nil }
     let(:recursive) { false }
     let(:pagination_params) { nil }
+    let(:skip_flat_paths) { false }
 
     let(:entries) { tree.first }
     let(:cursor) { tree.second }
@@ -107,6 +108,12 @@ RSpec.describe Gitlab::Git::Tree do
         end
 
         it { expect(subdir_file.flat_path).to eq('files/flat/path/correct') }
+
+        context 'when skip_flat_paths is true' do
+          let(:skip_flat_paths) { true }
+
+          it { expect(subdir_file.flat_path).to be_blank }
+        end
       end
     end
 
@@ -162,7 +169,7 @@ RSpec.describe Gitlab::Git::Tree do
         allow(instance).to receive(:lookup).with(SeedRepo::Commit::ID)
       end
 
-      described_class.where(repository, SeedRepo::Commit::ID, 'files', false)
+      described_class.where(repository, SeedRepo::Commit::ID, 'files', false, false)
     end
 
     it_behaves_like :repo do
@@ -180,7 +187,7 @@ RSpec.describe Gitlab::Git::Tree do
           let(:entries_count) { entries.count }
 
           it 'returns all entries without a cursor' do
-            result, cursor = Gitlab::Git::Tree.where(repository, sha, path, recursive, { limit: entries_count, page_token: nil })
+            result, cursor = Gitlab::Git::Tree.where(repository, sha, path, recursive, skip_flat_paths, { limit: entries_count, page_token: nil })
 
             expect(cursor).to be_nil
             expect(result.entries.count).to eq(entries_count)
@@ -209,7 +216,7 @@ RSpec.describe Gitlab::Git::Tree do
           let(:entries_count) { entries.count }
 
           it 'returns all entries' do
-            result, cursor = Gitlab::Git::Tree.where(repository, sha, path, recursive, { limit: -1, page_token: nil })
+            result, cursor = Gitlab::Git::Tree.where(repository, sha, path, recursive, skip_flat_paths, { limit: -1, page_token: nil })
 
             expect(result.count).to eq(entries_count)
             expect(cursor).to be_nil
@@ -220,7 +227,7 @@ RSpec.describe Gitlab::Git::Tree do
             let(:token) { entries.second.id }
 
             it 'returns all entries after token' do
-              result, cursor = Gitlab::Git::Tree.where(repository, sha, path, recursive, { limit: -1, page_token: token })
+              result, cursor = Gitlab::Git::Tree.where(repository, sha, path, recursive, skip_flat_paths, { limit: -1, page_token: token })
 
               expect(result.count).to eq(entries.count - 2)
               expect(cursor).to be_nil
@@ -252,7 +259,7 @@ RSpec.describe Gitlab::Git::Tree do
           expected_entries = entries
 
           loop do
-            result, cursor = Gitlab::Git::Tree.where(repository, sha, path, recursive, { limit: 5, page_token: token })
+            result, cursor = Gitlab::Git::Tree.where(repository, sha, path, recursive, skip_flat_paths, { limit: 5, page_token: token })
 
             collected_entries += result.entries
             token = cursor&.next_cursor
diff --git a/spec/lib/gitlab/gitaly_client/commit_service_spec.rb b/spec/lib/gitlab/gitaly_client/commit_service_spec.rb
index 0d591fe6c43..ed6a87cda6f 100644
--- a/spec/lib/gitlab/gitaly_client/commit_service_spec.rb
+++ b/spec/lib/gitlab/gitaly_client/commit_service_spec.rb
@@ -150,16 +150,18 @@ RSpec.describe Gitlab::GitalyClient::CommitService do
   end
 
   describe '#tree_entries' do
-    subject { client.tree_entries(repository, revision, path, recursive, pagination_params) }
+    subject { client.tree_entries(repository, revision, path, recursive, skip_flat_paths, pagination_params) }
 
     let(:path) { '/' }
     let(:recursive) { false }
     let(:pagination_params) { nil }
+    let(:skip_flat_paths) { false }
 
-    it 'sends a get_tree_entries message' do
+    it 'sends a get_tree_entries message with default limit' do
+      expected_pagination_params = Gitaly::PaginationParameter.new(limit: Gitlab::GitalyClient::CommitService::TREE_ENTRIES_DEFAULT_LIMIT)
       expect_any_instance_of(Gitaly::CommitService::Stub)
         .to receive(:get_tree_entries)
-        .with(gitaly_request_with_path(storage_name, relative_path), kind_of(Hash))
+        .with(gitaly_request_with_params({ pagination_params: expected_pagination_params }), kind_of(Hash))
         .and_return([])
 
       is_expected.to eq([[], nil])
@@ -189,9 +191,10 @@ RSpec.describe Gitlab::GitalyClient::CommitService do
           pagination_cursor: pagination_cursor
         )
 
+        expected_pagination_params = Gitaly::PaginationParameter.new(limit: 3)
         expect_any_instance_of(Gitaly::CommitService::Stub)
           .to receive(:get_tree_entries)
-          .with(gitaly_request_with_path(storage_name, relative_path), kind_of(Hash))
+          .with(gitaly_request_with_params({ pagination_params: expected_pagination_params }), kind_of(Hash))
           .and_return([response])
 
         is_expected.to eq([[], pagination_cursor])
diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb
index 530b03714b4..47532ed1216 100644
--- a/spec/models/repository_spec.rb
+++ b/spec/models/repository_spec.rb
@@ -2625,7 +2625,7 @@ RSpec.describe Repository do
   end
 
   shared_examples '#tree' do
-    subject { repository.tree(sha, path, recursive: recursive, pagination_params: pagination_params) }
+    subject { repository.tree(sha, path, recursive: recursive, skip_flat_paths: false, pagination_params: pagination_params) }
 
     let(:sha) { :head }
     let(:path) { nil }