summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
-rw-r--r--app/uploaders/file_uploader.rb7
-rw-r--r--lib/gitlab/gfm/uploads_rewriter.rb49
-rw-r--r--spec/lib/gitlab/gfm/uploads_rewriter_spec.rb29
-rw-r--r--spec/services/notes/copy_service_spec.rb6
4 files changed, 67 insertions, 24 deletions
diff --git a/app/uploaders/file_uploader.rb b/app/uploaders/file_uploader.rb
index bf5be708060..7250ce5c0b0 100644
--- a/app/uploaders/file_uploader.rb
+++ b/app/uploaders/file_uploader.rb
@@ -14,7 +14,12 @@ class FileUploader < GitlabUploader
include ObjectStorage::Concern
prepend ObjectStorage::Extension::RecordsUploads
- MARKDOWN_PATTERN = %r{\!?\[.*?\]\(/uploads/(?<secret>[0-9a-f]{32})/(?<file>.*?)\)}.freeze
+ # This pattern is vulnerable to malicious inputs, so use Gitlab::UntrustedRegexp
+ # to place bounds on execution time
+ MARKDOWN_PATTERN = Gitlab::UntrustedRegexp.new(
+ '!?\[.*?\]\(/uploads/(?P<secret>[0-9a-f]{32})/(?P<file>.*?)\)'
+ )
+
DYNAMIC_PATH_PATTERN = %r{.*(?<secret>\b(\h{10}|\h{32}))\/(?<identifier>.*)}.freeze
VALID_SECRET_PATTERN = %r{\A\h{10,32}\z}.freeze
diff --git a/lib/gitlab/gfm/uploads_rewriter.rb b/lib/gitlab/gfm/uploads_rewriter.rb
index b0bf68f4204..58b46a85aae 100644
--- a/lib/gitlab/gfm/uploads_rewriter.rb
+++ b/lib/gitlab/gfm/uploads_rewriter.rb
@@ -23,33 +23,24 @@ module Gitlab
def rewrite(target_parent)
return @text unless needs_rewrite?
- @text.gsub!(@pattern) do |markdown|
- file = find_file($~[:secret], $~[:file])
- # No file will be returned for a path traversal
- next if file.nil?
+ @target_parent = target_parent
- break markdown unless file.try(:exists?)
-
- klass = target_parent.is_a?(Namespace) ? NamespaceFileUploader : FileUploader
- moved = klass.copy_to(file, target_parent)
-
- moved_markdown = moved.markdown_link
-
- # Prevents rewrite of plain links as embedded
- if was_embedded?(markdown)
- moved_markdown
- else
- moved_markdown.delete_prefix('!')
- end
+ rewritten_text = Gitlab::StringRegexMarker.new(@text).mark(@pattern) do |markdown, left:, right:, mode:|
+ transform_markdown(markdown)
end
+
+ # MarkdownContentRewriterService relies on the text being changed _in place_.
+ @text.gsub!(@text, rewritten_text)
end
def needs_rewrite?
strong_memoize(:needs_rewrite) do
- FileUploader::MARKDOWN_PATTERN.match?(@text)
+ @pattern.match?(@text)
end
end
+ private
+
def was_embedded?(markdown)
markdown.starts_with?("!")
end
@@ -57,6 +48,28 @@ module Gitlab
def find_file(secret, file_name)
UploaderFinder.new(@source_project, secret, file_name).execute
end
+
+ def transform_markdown(markdown)
+ match = @pattern.match(markdown)
+ file = find_file(match[:secret], match[:file])
+
+ # No file will be returned for a path traversal
+ return '' if file.nil?
+
+ return markdown unless file.try(:exists?)
+
+ klass = @target_parent.is_a?(Namespace) ? NamespaceFileUploader : FileUploader
+ moved = klass.copy_to(file, @target_parent)
+
+ moved_markdown = moved.markdown_link
+
+ # Prevents rewrite of plain links as embedded
+ if was_embedded?(markdown)
+ moved_markdown
+ else
+ moved_markdown.delete_prefix('!')
+ end
+ end
end
end
end
diff --git a/spec/lib/gitlab/gfm/uploads_rewriter_spec.rb b/spec/lib/gitlab/gfm/uploads_rewriter_spec.rb
index a16f96a7d11..b1bff242f33 100644
--- a/spec/lib/gitlab/gfm/uploads_rewriter_spec.rb
+++ b/spec/lib/gitlab/gfm/uploads_rewriter_spec.rb
@@ -23,8 +23,9 @@ RSpec.describe Gitlab::Gfm::UploadsRewriter do
end
def referenced_files(text, project)
- referenced_files = text.scan(FileUploader::MARKDOWN_PATTERN).map do
- UploaderFinder.new(project, $~[:secret], $~[:file]).execute
+ scanner = FileUploader::MARKDOWN_PATTERN.scan(text)
+ referenced_files = scanner.map do |match|
+ UploaderFinder.new(project, match[0], match[1]).execute
end
referenced_files.compact.select(&:exists?)
@@ -32,7 +33,9 @@ RSpec.describe Gitlab::Gfm::UploadsRewriter do
shared_examples "files are accessible" do
describe '#rewrite' do
- let!(:new_text) { rewriter.rewrite(new_project) }
+ subject(:rewrite) { new_text }
+
+ let(:new_text) { rewriter.rewrite(new_project) }
let(:old_files) { [image_uploader, zip_uploader] }
let(:new_files) do
@@ -43,11 +46,15 @@ RSpec.describe Gitlab::Gfm::UploadsRewriter do
let(:new_paths) { new_files.map(&:path) }
it 'rewrites content' do
+ rewrite
+
expect(new_text).not_to eq text
expect(new_text.length).to eq text.length
end
it 'copies files' do
+ rewrite
+
expect(new_files).to all(exist)
expect(old_paths).not_to match_array new_paths
expect(old_paths).to all(include(old_project.disk_path))
@@ -55,10 +62,14 @@ RSpec.describe Gitlab::Gfm::UploadsRewriter do
end
it 'does not remove old files' do
+ rewrite
+
expect(old_files).to all(exist)
end
it 'generates a new secret for each file' do
+ rewrite
+
expect(new_paths).not_to include image_uploader.secret
expect(new_paths).not_to include zip_uploader.secret
end
@@ -68,6 +79,8 @@ RSpec.describe Gitlab::Gfm::UploadsRewriter do
allow(finder).to receive(:execute).and_return(nil)
end
+ rewrite
+
expect(new_files).to be_empty
end
end
@@ -84,6 +97,16 @@ RSpec.describe Gitlab::Gfm::UploadsRewriter do
expect(moved_text.scan(/\A\[.*?\]/).count).to eq(1)
end
+ it 'does not casue a timeout on pathological text' do
+ text = '[!l' * 30000
+
+ Timeout.timeout(3) do
+ moved_text = described_class.new(text, nil, old_project, user).rewrite(new_project)
+
+ expect(moved_text).to eq(text)
+ end
+ end
+
context "file are stored locally" do
include_examples "files are accessible"
end
diff --git a/spec/services/notes/copy_service_spec.rb b/spec/services/notes/copy_service_spec.rb
index f146a49e929..2fa9a462bb9 100644
--- a/spec/services/notes/copy_service_spec.rb
+++ b/spec/services/notes/copy_service_spec.rb
@@ -146,8 +146,10 @@ RSpec.describe Notes::CopyService do
new_note = to_noteable.notes.first
aggregate_failures do
- expect(note.note).to match(/Simple text with image: #{FileUploader::MARKDOWN_PATTERN}/o)
- expect(new_note.note).to match(/Simple text with image: #{FileUploader::MARKDOWN_PATTERN}/o)
+ expect(note.note).to match(/Simple text with image:/o)
+ expect(FileUploader::MARKDOWN_PATTERN.match(note.note)).not_to be_nil
+ expect(new_note.note).to match(/Simple text with image:/o)
+ expect(FileUploader::MARKDOWN_PATTERN.match(new_note.note)).not_to be_nil
expect(note.note).not_to eq(new_note.note)
expect(note.note_html).not_to eq(new_note.note_html)
end