From 701584e24e5c19ccc2be5f1b39a7bedca40e816a Mon Sep 17 00:00:00 2001
From: Mike Perham <mperham@gmail.com>
Date: Fri, 24 Mar 2017 11:56:10 -0700
Subject: [PATCH] Verify graceful handling of ill-formed job payloads, fixes
 #3406

Sidekiq will better handle jobs with malformed payloads.  Any job which raises a JSON::ParserError will immediately move to the Dead set.  Update the API to degrade gracefully when trying to render bad JSON in the queue, scheduled or dead.  These payloads are most often from other languages where the JSON is being pieced together manually and pushed to Redis.
---
 5.0-Upgrade.md     |  7 ++++---
 lib/sidekiq/api.rb | 24 ++++++++++++++++--------
 test/test_web.rb   | 27 +++++++++++++++++++++++++++
 3 files changed, 47 insertions(+), 11 deletions(-)

diff --git a/5.0-Upgrade.md b/5.0-Upgrade.md
index 234d4151..e45a94f0 100644
--- a/5.0-Upgrade.md
+++ b/5.0-Upgrade.md
@@ -33,11 +33,12 @@ versions of Ruby and Rails and adds support for RTL languages in the Web UI.
 * The Web UI is now bi-directional - it can render either LTR
   (left-to-right) or RTL languages. With this change, **Farsi, Arabic,
   Hebrew and Urdu** are officially supported. [#3381]
-* Rails 3.2 is no longer supported.
-* Ruby 2.0 and Ruby 2.1 are no longer supported. Ruby 2.2.2+ is required.
 * Jobs which can't be parsed due to invalid JSON are now pushed
   immediately to the Dead set since they require manual intervention and
-  will never execute successfully as is. [#3296]
+  will never execute successfully as is.  The Web UI has been updated to
+  more gracefully display these jobs. [#3296]
+* **Rails 3.2** is no longer supported.
+* **Ruby 2.0 and Ruby 2.1** are no longer supported. Ruby 2.2.2+ is required.
 
 ## Upgrade
 
diff --git a/lib/sidekiq/api.rb b/lib/sidekiq/api.rb
index 68184815..f4a52d15 100644
--- a/lib/sidekiq/api.rb
+++ b/lib/sidekiq/api.rb
@@ -75,7 +75,7 @@ module Sidekiq
       enqueued     = pipe2_res[s..-1].map(&:to_i).inject(0, &:+)
 
       default_queue_latency = if (entry = pipe1_res[6].first)
-                                job = Sidekiq.load_json(entry)
+                                job = Sidekiq.load_json(entry) rescue {}
                                 now = Time.now.to_f
                                 thence = job['enqueued_at'.freeze] || now
                                 now - thence
@@ -287,13 +287,21 @@ module Sidekiq
     attr_reader :value
 
     def initialize(item, queue_name=nil)
+      @args = nil
       @value = item
-      @item = if item.is_a?(Hash)
-                item
-              else
-                Sidekiq.load_json(item) rescue nil
-              end
-      @queue = queue_name || self['queue']
+      @item = item.is_a?(Hash) ? item : parse(item)
+      @queue = queue_name || @item['queue']
+    end
+
+    def parse(item)
+      Sidekiq.load_json(item)
+    rescue JSON::ParserError
+      # If the job payload in Redis is invalid JSON, we'll load
+      # the item as an empty hash and store the invalid JSON as
+      # the job 'args' for display in the Web UI.
+      @invalid = true
+      @args = [item]
+      {}
     end
 
     def klass
@@ -341,7 +349,7 @@ module Sidekiq
     end
 
     def args
-      self['args']
+      @args || @item['args']
     end
 
     def jid
diff --git a/test/test_web.rb b/test/test_web.rb
index 4084e28b..c877ec57 100644
--- a/test/test_web.rb
+++ b/test/test_web.rb
@@ -498,6 +498,24 @@ class TestWeb < Sidekiq::Test
       end
     end
 
+    describe 'bad JSON' do
+      it 'displays without error' do
+        s = Sidekiq::DeadSet.new
+        (_, score) = kill_bad
+        assert_equal 1, s.size
+
+        get '/morgue'
+        assert_equal 200, last_response.status
+        assert_match(/#{score.to_i}/, last_response.body)
+        assert_match("something bad", last_response.body)
+        assert_equal 1, s.size
+
+        post "/morgue/#{score}-", 'delete' => 'Delete'
+        assert_equal 302, last_response.status
+        assert_equal 0, s.size
+      end
+    end
+
     describe 'stats/queues' do
       include Sidekiq::Util
 
@@ -616,6 +634,15 @@ class TestWeb < Sidekiq::Test
       [msg, score]
     end
 
+    def kill_bad
+      job = "{ something bad }"
+      score = Time.now.to_f
+      Sidekiq.redis do |conn|
+        conn.zadd('schedule', score, job)
+      end
+      [job, score]
+    end
+
     def add_xss_retry(job_id=SecureRandom.hex(12))
       msg = { 'class' => 'FailWorker',
               'args' => ['<a>hello</a>'],