summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorGitLab Bot <gitlab-bot@gitlab.com>2022-08-26 14:37:09 +0000
committerGitLab Bot <gitlab-bot@gitlab.com>2022-08-26 14:37:20 +0000
commit25ed7b6ae4712518e96d4719b75dd293c57404a2 (patch)
tree102e02b15909f27a82b6cf64e7b878f0b201b383
parentdaf5ae5bd439f1f32363d410129d5b9e73fbb539 (diff)
downloadgitlab-ce-25ed7b6ae4712518e96d4719b75dd293c57404a2.tar.gz
Add latest changes from gitlab-org/security/gitlab@15-3-stable-ee
-rw-r--r--app/graphql/resolvers/paginated_tree_resolver.rb6
-rw-r--r--app/models/repository.rb10
-rw-r--r--app/models/tree.rb4
-rw-r--r--lib/banzai/filter/commit_trailers_filter.rb34
-rw-r--r--lib/banzai/filter/pathological_markdown_filter.rb27
-rw-r--r--lib/banzai/pipeline/plain_markdown_pipeline.rb1
-rw-r--r--lib/gitlab/git/rugged_impl/tree.rb9
-rw-r--r--lib/gitlab/git/tree.rb9
-rw-r--r--lib/gitlab/gitaly_client/commit_service.rb8
-rw-r--r--spec/lib/banzai/filter/commit_trailers_filter_spec.rb25
-rw-r--r--spec/lib/banzai/filter/pathological_markdown_filter_spec.rb27
-rw-r--r--spec/lib/banzai/pipeline/full_pipeline_spec.rb12
-rw-r--r--spec/lib/gitlab/git/tree_spec.rb19
-rw-r--r--spec/lib/gitlab/gitaly_client/commit_service_spec.rb11
-rw-r--r--spec/models/repository_spec.rb2
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 }