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

app/graphql/resolvers/paginated_tree_resolver.rb

@@ -32,7 +32,11 @@ def resolve(**args)
         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)

app/models/repository.rb

@@ -677,24 +677,24 @@ def head_commit
     @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)

app/models/tree.rb

@@ -6,15 +6,15 @@ 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
     @sha = sha
     @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

lib/banzai/filter/commit_trailers_filter.rb

@@ -17,21 +17,10 @@ class CommitTrailersFilter < HTML::Pipeline::Filter
       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 @@ def call
       # 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 @@ def trailer_filter(text)
       # 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,

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

lib/banzai/pipeline/plain_markdown_pipeline.rb

@@ -5,6 +5,7 @@ module Pipeline
     class PlainMarkdownPipeline < BasePipeline
       def self.filters
         FilterArray[
+          Filter::PathologicalMarkdownFilter,
           Filter::MarkdownPreEscapeFilter,
           Filter::MarkdownFilter,
           Filter::MarkdownPostEscapeFilter

lib/gitlab/git/rugged_impl/tree.rb

@@ -16,9 +16,10 @@ module ClassMethods
           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 @@ def paginated_response(entries, limit, token)
             [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
 

lib/gitlab/git/tree.rb

@@ -15,15 +15,16 @@ class << self
         # 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
 

lib/gitlab/gitaly_client/commit_service.rb

@@ -5,6 +5,8 @@ module GitalyClient
     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 @@ def tree_entry(ref, path, limit = nil)
         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

spec/lib/banzai/filter/commit_trailers_filter_spec.rb

@@ -18,10 +18,20 @@
   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 @@
 
   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)

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

spec/lib/banzai/pipeline/full_pipeline_spec.rb

@@ -167,4 +167,16 @@
       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

spec/lib/gitlab/git/tree_spec.rb

@@ -9,12 +9,13 @@
   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 @@
         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 @@
         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 @@
           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 @@
           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 @@
             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 @@
           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