Merge branch 'blackst0ne-bump-rails-cve-2018-16476' into 'master'

Bump Ruby on Rails to 5.0.7.1

See merge request gitlab-org/gitlab-ce!23396

Gemfile

@@ -1,6 +1,6 @@
 source 'https://rubygems.org'
 
-gem 'rails', '5.0.7'
+gem 'rails', '5.0.7.1'
 gem 'rails-deprecated_sanitizer', '~> 1.0.3'
 
 # Improves copy-on-write performance for MRI

Gemfile.lock

@@ -4,41 +4,41 @@ GEM
     RedCloth (4.3.2)
     abstract_type (0.0.7)
     ace-rails-ap (4.1.2)
-    actioncable (5.0.7)
-      actionpack (= 5.0.7)
+    actioncable (5.0.7.1)
+      actionpack (= 5.0.7.1)
       nio4r (>= 1.2, < 3.0)
       websocket-driver (~> 0.6.1)
-    actionmailer (5.0.7)
-      actionpack (= 5.0.7)
-      actionview (= 5.0.7)
-      activejob (= 5.0.7)
+    actionmailer (5.0.7.1)
+      actionpack (= 5.0.7.1)
+      actionview (= 5.0.7.1)
+      activejob (= 5.0.7.1)
       mail (~> 2.5, >= 2.5.4)
       rails-dom-testing (~> 2.0)
-    actionpack (5.0.7)
-      actionview (= 5.0.7)
-      activesupport (= 5.0.7)
+    actionpack (5.0.7.1)
+      actionview (= 5.0.7.1)
+      activesupport (= 5.0.7.1)
       rack (~> 2.0)
       rack-test (~> 0.6.3)
       rails-dom-testing (~> 2.0)
       rails-html-sanitizer (~> 1.0, >= 1.0.2)
-    actionview (5.0.7)
-      activesupport (= 5.0.7)
+    actionview (5.0.7.1)
+      activesupport (= 5.0.7.1)
       builder (~> 3.1)
       erubis (~> 2.7.0)
       rails-dom-testing (~> 2.0)
       rails-html-sanitizer (~> 1.0, >= 1.0.3)
-    activejob (5.0.7)
-      activesupport (= 5.0.7)
+    activejob (5.0.7.1)
+      activesupport (= 5.0.7.1)
       globalid (>= 0.3.6)
-    activemodel (5.0.7)
-      activesupport (= 5.0.7)
-    activerecord (5.0.7)
-      activemodel (= 5.0.7)
-      activesupport (= 5.0.7)
+    activemodel (5.0.7.1)
+      activesupport (= 5.0.7.1)
+    activerecord (5.0.7.1)
+      activemodel (= 5.0.7.1)
+      activesupport (= 5.0.7.1)
       arel (~> 7.0)
     activerecord_sane_schema_dumper (1.0)
       rails (>= 5, < 6)
-    activesupport (5.0.7)
+    activesupport (5.0.7.1)
       concurrent-ruby (~> 1.0, >= 1.0.2)
       i18n (>= 0.7, < 2)
       minitest (~> 5.1)
@@ -381,7 +381,7 @@ GEM
       json (~> 1.8)
       multi_xml (>= 0.5.2)
     httpclient (2.8.3)
-    i18n (1.1.1)
+    i18n (1.2.0)
       concurrent-ruby (~> 1.0)
     icalendar (2.4.1)
     ice_nine (0.11.2)
@@ -449,7 +449,7 @@ GEM
     loofah (2.2.3)
       crass (~> 1.0.2)
       nokogiri (>= 1.5.9)
-    mail (2.7.0)
+    mail (2.7.1)
       mini_mime (>= 0.1.1)
     mail_room (0.9.1)
     memoist (0.16.0)
@@ -624,17 +624,17 @@ GEM
       rack
     rack-test (0.6.3)
       rack (>= 1.0)
-    rails (5.0.7)
-      actioncable (= 5.0.7)
-      actionmailer (= 5.0.7)
-      actionpack (= 5.0.7)
-      actionview (= 5.0.7)
-      activejob (= 5.0.7)
-      activemodel (= 5.0.7)
-      activerecord (= 5.0.7)
-      activesupport (= 5.0.7)
+    rails (5.0.7.1)
+      actioncable (= 5.0.7.1)
+      actionmailer (= 5.0.7.1)
+      actionpack (= 5.0.7.1)
+      actionview (= 5.0.7.1)
+      activejob (= 5.0.7.1)
+      activemodel (= 5.0.7.1)
+      activerecord (= 5.0.7.1)
+      activesupport (= 5.0.7.1)
       bundler (>= 1.3.0)
-      railties (= 5.0.7)
+      railties (= 5.0.7.1)
       sprockets-rails (>= 2.0.0)
     rails-controller-testing (1.0.2)
       actionpack (~> 5.x, >= 5.0.1)
@@ -650,15 +650,15 @@ GEM
     rails-i18n (5.1.1)
       i18n (>= 0.7, < 2)
       railties (>= 5.0, < 6)
-    railties (5.0.7)
-      actionpack (= 5.0.7)
-      activesupport (= 5.0.7)
+    railties (5.0.7.1)
+      actionpack (= 5.0.7.1)
+      activesupport (= 5.0.7.1)
       method_source
       rake (>= 0.8.7)
       thor (>= 0.18.1, < 2.0)
     rainbow (3.0.0)
     raindrops (0.18.0)
-    rake (12.3.1)
+    rake (12.3.2)
     rb-fsevent (0.10.2)
     rb-inotify (0.9.10)
       ffi (>= 0.5.0, < 2)
@@ -1096,7 +1096,7 @@ DEPENDENCIES
   rack-cors (~> 1.0.0)
   rack-oauth2 (~> 1.2.1)
   rack-proxy (~> 0.6.0)
-  rails (= 5.0.7)
+  rails (= 5.0.7.1)
   rails-controller-testing
   rails-deprecated_sanitizer (~> 1.0.3)
   rails-i18n (~> 5.1)

app/services/merge_requests/update_service.rb

@@ -46,11 +46,13 @@ module MergeRequests
       end
 
       if merge_request.previous_changes.include?('assignee_id')
+        reassigned_merge_request_args = [merge_request, current_user]
+
         old_assignee_id = merge_request.previous_changes['assignee_id'].first
-        old_assignee = User.find(old_assignee_id) if old_assignee_id
+        reassigned_merge_request_args << User.find(old_assignee_id) if old_assignee_id
 
         create_assignee_note(merge_request)
-        notification_service.async.reassigned_merge_request(merge_request, current_user, old_assignee)
+        notification_service.async.reassigned_merge_request(*reassigned_merge_request_args)
         todo_service.reassigned_merge_request(merge_request, current_user)
       end
 

app/services/notification_service.rb

@@ -188,7 +188,7 @@ class NotificationService
   #  * merge_request assignee if their notification level is not Disabled
   #  * users with custom level checked with "reassign merge request"
   #
-  def reassigned_merge_request(merge_request, current_user, previous_assignee)
+  def reassigned_merge_request(merge_request, current_user, previous_assignee = nil)
     recipients = NotificationRecipientService.build_recipients(
       merge_request,
       current_user,

app/workers/mail_scheduler/notification_service_worker.rb

@@ -8,14 +8,35 @@ module MailScheduler
     include MailSchedulerQueue
 
     def perform(meth, *args)
-      deserialized_args = ActiveJob::Arguments.deserialize(args)
+      check_arguments!(args)
 
+      deserialized_args = ActiveJob::Arguments.deserialize(args)
       notification_service.public_send(meth, *deserialized_args) # rubocop:disable GitlabSecurity/PublicSend
     rescue ActiveJob::DeserializationError
+      # No-op.
+      # This exception gets raised when an argument
+      # is correct (deserializeable), but it still cannot be deserialized.
+      # This can happen when an object has been deleted after
+      # rails passes this job to sidekiq, but before
+      # sidekiq gets it for execution.
+      # In this case just do nothing.
     end
 
     def self.perform_async(*args)
       super(*ActiveJob::Arguments.serialize(args))
     end
+
+    private
+
+    # If an argument is in the ActiveJob::Arguments::TYPE_WHITELIST list,
+    # it means the argument cannot be deserialized.
+    # Which means there's something wrong with our code.
+    def check_arguments!(args)
+      args.each do |arg|
+        if arg.class.in?(ActiveJob::Arguments::TYPE_WHITELIST)
+          raise(ArgumentError, "Argument `#{arg}` cannot be deserialized because of its type")
+        end
+      end
+    end
   end
 end

changelogs/unreleased/blackst0ne-bump-rails-cve-2018-16476.yml

@@ -0,0 +1,5 @@
+---
+title: Bump Ruby on Rails to 5.0.7.1
+merge_request: 23396
+author: "@blackst0ne"
+type: security

spec/workers/mail_scheduler/notification_service_worker_spec.rb

@@ -17,10 +17,21 @@ describe MailScheduler::NotificationServiceWorker do
     end
 
     context 'when the arguments cannot be deserialized' do
-      it 'does nothing' do
-        expect(worker.notification_service).not_to receive(method)
+      context 'when the arguments are not deserializeable' do
+        it 'raises exception' do
+          expect(worker.notification_service).not_to receive(method)
+          expect { worker.perform(method, key.to_global_id.to_s.succ) }.to raise_exception(ArgumentError)
+        end
+      end
 
-        worker.perform(method, key.to_global_id.to_s.succ)
+      context 'when the arguments are deserializeable' do
+        it 'does nothing' do
+          serialized_arguments = *serialize(key)
+          key.destroy!
+
+          expect(worker.notification_service).not_to receive(method)
+          expect { worker.perform(method, serialized_arguments) }.not_to raise_exception
+        end
       end
     end