diff options
| -rw-r--r-- | app/models/hooks/web_hook_log.rb | 10 | ||||
| -rw-r--r-- | app/uploaders/file_uploader.rb | 7 | ||||
| -rw-r--r-- | lib/gitlab/ci/ansi2json/line.rb | 3 | ||||
| -rw-r--r-- | lib/gitlab/gfm/uploads_rewriter.rb | 49 | ||||
| -rw-r--r-- | spec/lib/gitlab/ci/ansi2json/line_spec.rb | 2 | ||||
| -rw-r--r-- | spec/lib/gitlab/gfm/uploads_rewriter_spec.rb | 29 | ||||
| -rw-r--r-- | spec/models/hooks/web_hook_log_spec.rb | 47 | ||||
| -rw-r--r-- | spec/services/notes/copy_service_spec.rb | 6 |
8 files changed, 111 insertions, 42 deletions
diff --git a/app/models/hooks/web_hook_log.rb b/app/models/hooks/web_hook_log.rb index 24e5f193a32..82ae71213da 100644 --- a/app/models/hooks/web_hook_log.rb +++ b/app/models/hooks/web_hook_log.rb @@ -22,7 +22,7 @@ class WebHookLog < ApplicationRecord validates :web_hook, presence: true before_save :obfuscate_basic_auth - before_save :redact_author_email + before_save :redact_user_emails def self.recent where('created_at >= ?', 2.days.ago.beginning_of_day) @@ -54,9 +54,9 @@ class WebHookLog < ApplicationRecord self.url = safe_url end - def redact_author_email - return unless self.request_data.dig('commit', 'author', 'email').present? - - self.request_data['commit']['author']['email'] = _('[REDACTED]') + def redact_user_emails + self.request_data.deep_transform_values! do |value| + value =~ URI::MailTo::EMAIL_REGEXP ? _('[REDACTED]') : value + end end end 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/ci/ansi2json/line.rb b/lib/gitlab/ci/ansi2json/line.rb index e48080993ab..abe2f272ca7 100644 --- a/lib/gitlab/ci/ansi2json/line.rb +++ b/lib/gitlab/ci/ansi2json/line.rb @@ -80,7 +80,8 @@ module Gitlab end def set_section_duration(duration_in_seconds) - duration = ActiveSupport::Duration.build(duration_in_seconds.to_i) + normalized_duration_in_seconds = duration_in_seconds.to_i.clamp(0, 1.year) + duration = ActiveSupport::Duration.build(normalized_duration_in_seconds) hours = duration.in_hours.floor hours = hours > 0 ? "%02d" % hours : nil minutes = "%02d" % duration.parts[:minutes].to_i 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/ci/ansi2json/line_spec.rb b/spec/lib/gitlab/ci/ansi2json/line_spec.rb index d16750d19f1..b8563bb1d1c 100644 --- a/spec/lib/gitlab/ci/ansi2json/line_spec.rb +++ b/spec/lib/gitlab/ci/ansi2json/line_spec.rb @@ -87,6 +87,8 @@ RSpec.describe Gitlab::Ci::Ansi2json::Line do 1.minute + 15.seconds | '01:15' 13.hours + 14.minutes + 15.seconds | '13:14:15' 1.day + 13.hours + 14.minutes + 15.seconds | '37:14:15' + Float::MAX | '8765:00:00' + 10**10000 | '8765:00:00' end with_them do diff --git a/spec/lib/gitlab/gfm/uploads_rewriter_spec.rb b/spec/lib/gitlab/gfm/uploads_rewriter_spec.rb index 763e6f1b5f4..5ce7afa6200 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/models/hooks/web_hook_log_spec.rb b/spec/models/hooks/web_hook_log_spec.rb index 8ff8a1c3865..3441dfda7d6 100644 --- a/spec/models/hooks/web_hook_log_spec.rb +++ b/spec/models/hooks/web_hook_log_spec.rb @@ -44,26 +44,49 @@ RSpec.describe WebHookLog do end end - context 'with author email' do + context "with users' emails" do let(:author) { create(:user) } + let(:user) { create(:user) } let(:web_hook_log) { create(:web_hook_log, request_data: data) } let(:data) do { - commit: { - author: { - name: author.name, - email: author.email + user: { + name: user.name, + email: user.email + }, + commits: [ + { + user: { + name: author.name, + email: author.email + } + }, + { + user: { + name: user.name, + email: user.email + } } - } + ] }.deep_stringify_keys end - it "redacts author's email" do - expect(web_hook_log.request_data['commit']).to match a_hash_including( - 'author' => { - 'name' => author.name, - 'email' => _('[REDACTED]') - } + it "redacts users' emails" do + expect(web_hook_log.request_data['user']).to match a_hash_including( + 'name' => user.name, + 'email' => _('[REDACTED]') + ) + expect(web_hook_log.request_data['commits'].pluck('user')).to match_array( + [ + { + 'name' => author.name, + 'email' => _('[REDACTED]') + }, + { + 'name' => user.name, + 'email' => _('[REDACTED]') + } + ] ) end 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 |