From fde53a5fc12b4a72d23c58d0854d41b2d15f38e0 Mon Sep 17 00:00:00 2001 From: Kelly Sutton Date: Tue, 7 Dec 2021 13:20:20 -0800 Subject: [PATCH] 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 * Swap out JSON.load for JSON.parse per the security CI check Co-authored-by: Eda Zhou * Add a few more tests cases to build up confidence around our JSON.parse/dump approach and deep structures Co-authored-by: Eda Zhou * 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 * Refactor a few tests to test a single argument at a time Co-authored-by: Eda Zhou * Break out test cases to exercise each individual intersting case instead of all at once Co-authored-by: Eda Zhou * Change formatting to be more consistent, tighter when arguments are simple Co-authored-by: Eda Zhou * Add a flag to disable the warning message for development warning messages Co-authored-by: Eda Zhou Co-authored-by: Eda Zhou --- lib/sidekiq.rb | 4 ++ lib/sidekiq/job_util.rb | 23 ++++++++- test/test_client.rb | 104 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 130 insertions(+), 1 deletion(-) diff --git a/lib/sidekiq.rb b/lib/sidekiq.rb index 92d6cf2e..1e5527da 100644 --- a/lib/sidekiq.rb +++ b/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 diff --git a/lib/sidekiq/job_util.rb b/lib/sidekiq/job_util.rb index e9a14cc2..75af6f4d 100644 --- a/lib/sidekiq/job_util.rb +++ b/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 diff --git a/test/test_client.rb b/test/test_client.rb index f9d39955..550e471e 100644 --- a/test/test_client.rb +++ b/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