Implement strict argument checking (#5071)

* Add the outline of failing tests

* Implement argument checking

* Move argument checking into Sidekiq::JobUtil#validate

* Refactor acceptable class definition into a constant to cut down on array allocations

* Improve error message, match raise call formatting to other errors in the class

* Address feedback in the Pull Request to use the JSON round-trip method
of confirming the safety of job argument payloads.

Cleanup commented-out code from a few years back.

Co-authored-by: Eda Zhou <eda.zhou@gusto.com>

* Swap out JSON.load for JSON.parse per the security CI check

Co-authored-by: Eda Zhou <eda.zhou@gusto.com>

* Add a few more tests cases to build up confidence around our JSON.parse/dump approach and deep structures

Co-authored-by: Eda Zhou <eda.zhou@gusto.com>

* Improve test case description

* Warn when job arguments do not serialize safely and point folks toward how to enable strict_mode and the best practice

* Reconfigure the options-hash based approach to a global Sidekiq.strict_mode! method

* Add a note in the raised error on how to disable the error

* Let the error message breathe a little bit

* Toggle strict_mode! off to suss out a test flake

* Capitalize the start of a sentence

* Rename job_is_json_safe to json_safe?

Co-authored-by: Eda Zhou <eda.zhou@gusto.com>

* Refactor a few tests to test a single argument at a time

Co-authored-by: Eda Zhou <eda.zhou@gusto.com>

* Break out test cases to exercise each individual intersting case instead of all at once

Co-authored-by: Eda Zhou <eda.zhou@gusto.com>

* Change formatting to be more consistent, tighter when arguments are simple

Co-authored-by: Eda Zhou <eda.zhou@gusto.com>

* Add a flag to disable the warning message for development warning messages

Co-authored-by: Eda Zhou <eda.zhou@gusto.com>

Co-authored-by: Eda Zhou <eda.zhou@gusto.com>

lib/sidekiq.rb

@@ -252,6 +252,10 @@ module Sidekiq
     options[:lifecycle_events][event] << block
   end
 
+  def self.strict_mode!(val = true)
+    options[:raise_on_complex_arguments] = val
+  end
+
   # We are shutting down Sidekiq but what about workers that
   # are working on some long job?  This error is
   # raised in workers that have not finished within the hard

lib/sidekiq/job_util.rb

@@ -12,11 +12,26 @@ module Sidekiq
       raise(ArgumentError, "Job class must be either a Class or String representation of the class name: `#{item}`") unless item["class"].is_a?(Class) || item["class"].is_a?(String)
       raise(ArgumentError, "Job 'at' must be a Numeric timestamp: `#{item}`") if item.key?("at") && !item["at"].is_a?(Numeric)
       raise(ArgumentError, "Job tags must be an Array: `#{item}`") if item["tags"] && !item["tags"].is_a?(Array)
+
+      if Sidekiq.options[:raise_on_complex_arguments] && !json_safe?(item)
+        msg = <<~EOM
+          Arguments must be native JSON types, see https://github.com/mperham/sidekiq/wiki/Best-Practices.
+
+          To disable this error, remove `Sidekiq.strict_mode!` from your initializer.
+        EOM
+        raise(ArgumentError, msg)
+      elsif !Sidekiq.options[:disable_complex_argument_warning] && Sidekiq.options[:environment] == "development" && !json_safe?(item)
+        Sidekiq.logger.warn <<~EOM
+          Job arguments do not serialize to JSON safely. This will raise an error in Sidekiq 7.0.
+
+          See https://github.com/mperham/sidekiq/wiki/Best-Practices or raise the error today
+          by calling `Sidekiq.strict_mode!` during Sidekiq initialization.
+        EOM
+      end
     end
 
     def normalize_item(item)
       validate(item)
-      # raise(ArgumentError, "Arguments must be native JSON types, see https://github.com/mperham/sidekiq/wiki/Best-Practices") unless JSON.load(JSON.dump(item['args'])) == item['args']
 
       # merge in the default sidekiq_options for the item's class and/or wrapped element
       # this allows ActiveJobs to control sidekiq_options too.
@@ -42,5 +57,11 @@ module Sidekiq
         Sidekiq.default_worker_options
       end
     end
+
+    private
+
+    def json_safe?(item)
+      JSON.parse(JSON.dump(item['args'])) == item['args']
+    end
   end
 end

test/test_client.rb

@@ -122,6 +122,110 @@ describe Sidekiq::Client do
       assert QueuedWorker.perform_async(1, 2)
       assert_equal 1, Sidekiq::Queue.new('flimflam').size
     end
+
+    describe 'argument checking' do
+      class InterestingWorker
+        include Sidekiq::Worker
+
+        def perform(an_argument)
+        end
+      end
+
+      it 'enqueues jobs with a symbol as an argument' do
+        InterestingWorker.perform_async(:symbol)
+      end
+
+      it 'enqueues jobs with a Date as an argument' do
+        InterestingWorker.perform_async(Date.new(2021, 1, 1))
+      end
+
+      it 'enqueues jobs with a Hash with symbols and string as keys as an argument' do
+        InterestingWorker.perform_async(
+          {
+            some: 'hash',
+            'with' => 'different_keys'
+          }
+        )
+      end
+
+      it 'enqueues jobs with a Struct as an argument' do
+        InterestingWorker.perform_async(
+          Struct.new(:x, :y).new(0, 0)
+        )
+      end
+
+      it 'works with a JSON-friendly deep, nested structure' do
+        InterestingWorker.perform_async(
+          {
+            'foo' => ['a', 'b', 'c'],
+            'bar' => ['x', 'y', 'z']
+          }
+        )
+      end
+
+      describe 'strict mode is enabled' do
+        before do
+          Sidekiq.strict_mode!
+        end
+
+        after do
+          Sidekiq.strict_mode!(false)
+        end
+
+        it 'raises an error when using a symbol as an argument' do
+          assert_raises ArgumentError do
+            InterestingWorker.perform_async(:symbol)
+          end
+        end
+
+        it 'raises an error when using a Date as an argument' do
+          assert_raises ArgumentError do
+            InterestingWorker.perform_async(Date.new(2021, 1, 1))
+          end
+        end
+
+        it 'raises an error when using a Hash with symbols and string as keys as an argument' do
+          assert_raises ArgumentError do
+            InterestingWorker.perform_async(
+              {
+                some: 'hash',
+                'with' => 'different_keys'
+              }
+            )
+          end
+        end
+
+        it 'raises an error when using a Struct as an argument' do
+          assert_raises ArgumentError do
+            InterestingWorker.perform_async(
+              Struct.new(:x, :y).new(0, 0)
+            )
+          end
+        end
+
+        it 'works with a JSON-friendly deep, nested structure' do
+          InterestingWorker.perform_async(
+            {
+              'foo' => ['a', 'b', 'c'],
+              'bar' => ['x', 'y', 'z']
+            }
+          )
+        end
+
+        describe 'worker that takes deep, nested structures' do
+          it 'raises an error on JSON-unfriendly structures' do
+            assert_raises ArgumentError do
+              InterestingWorker.perform_async(
+                {
+                  'foo' => [:a, :b, :c],
+                  bar: ['x', 'y', 'z']
+                }
+              )
+            end
+          end
+        end
+      end
+    end
   end
 
   describe 'bulk' do