Merge pull request #595 from bugsnag/next

Release v6.13.1

CHANGELOG.md

@@ -1,6 +1,16 @@
 Changelog
 =========
 
+## 6.13.1 (11 May 2020)
+
+### Fixes
+
+* Only call custom diagnostic data methods once
+  | [#586](https://github.com/bugsnag/bugsnag-ruby/pull/586)
+  | [stoivo](https://github.com/stoivo)
+* Guard against exceptions in to_s when cleaning objects
+  | [#591](https://github.com/bugsnag/bugsnag-ruby/pull/591)
+
 ## 6.13.0 (30 Jan 2020)
 
 ### Enhancements

VERSION

@@ -1 +1 @@
-6.13.0
+6.13.1

lib/bugsnag/cleaner.rb

@@ -29,10 +29,19 @@ def traverse_object(obj, seen, scope)
       when Hash
         clean_hash = {}
         obj.each do |k,v|
-          if filters_match_deeply?(k, scope)
-            clean_hash[k] = FILTERED
-          else
-            clean_hash[k] = traverse_object(v, seen, [scope, k].compact.join('.'))
+          begin
+            if filters_match_deeply?(k, scope)
+              clean_hash[k] = FILTERED
+            else
+              clean_hash[k] = traverse_object(v, seen, [scope, k].compact.join('.'))
+            end
+          # If we get an error here, we assume the key needs to be filtered
+          # to avoid leaking things we shouldn't. We also remove the key itself
+          # because it may cause issues later e.g. when being converted to JSON
+          rescue StandardError
+            clean_hash[RAISED] = FILTERED
+          rescue SystemStackError
+            clean_hash[RECURSION] = FILTERED
           end
         end
         clean_hash
@@ -43,7 +52,15 @@ def traverse_object(obj, seen, scope)
       when String
         clean_string(obj)
       else
-        str = obj.to_s rescue RAISED
+        # guard against objects that raise or blow the stack when stringified
+        begin
+          str = obj.to_s
+        rescue StandardError
+          str = RAISED
+        rescue SystemStackError
+          str = RECURSION
+        end
+
         # avoid leaking potentially sensitive data from objects' #inspect output
         if str =~ /#<.*>/
           OBJECT

lib/bugsnag/middleware/exception_meta_data.rb

@@ -9,21 +9,27 @@ def initialize(bugsnag)
     def call(report)
       # Apply the user's information attached to the exceptions
       report.raw_exceptions.each do |exception|
-        if exception.respond_to?(:bugsnag_user_id) && exception.bugsnag_user_id.is_a?(String)
-          report.user = {id: exception.bugsnag_user_id}
+        if exception.respond_to?(:bugsnag_user_id)
+          user_id = exception.bugsnag_user_id
+          report.user = {id: user_id} if user_id.is_a?(String)
         end
 
-        if exception.respond_to?(:bugsnag_context) && exception.bugsnag_context.is_a?(String)
-          report.context = exception.bugsnag_context
+        if exception.respond_to?(:bugsnag_context)
+          context = exception.bugsnag_context
+          report.context = context if context.is_a?(String)
         end
 
-        if exception.respond_to?(:bugsnag_grouping_hash) && exception.bugsnag_grouping_hash.is_a?(String)
-          report.grouping_hash = exception.bugsnag_grouping_hash
+        if exception.respond_to?(:bugsnag_grouping_hash)
+          group_hash = exception.bugsnag_grouping_hash
+          report.grouping_hash = group_hash if group_hash.is_a?(String)
         end
 
-        if exception.respond_to?(:bugsnag_meta_data) && exception.bugsnag_meta_data.is_a?(Hash)
-          exception.bugsnag_meta_data.each do |key, value|
-            report.add_tab key, value
+        if exception.respond_to?(:bugsnag_meta_data)
+          meta_data = exception.bugsnag_meta_data
+          if meta_data.is_a?(Hash)
+            meta_data.each do |key, value|
+              report.add_tab key, value
+            end
           end
         end
       end

spec/cleaner_spec.rb

@@ -6,12 +6,84 @@
   subject { described_class.new(nil) }
 
   describe "#clean_object" do
+    is_jruby = defined?(RUBY_ENGINE) && RUBY_ENGINE == 'jruby'
+
     it "cleans up recursive hashes" do
       a = {:a => {}}
       a[:a][:b] = a
       expect(subject.clean_object(a)).to eq({:a => {:b => "[RECURSION]"}})
     end
 
+    it "cleans up hashes when keys infinitely recurse in to_s" do
+      skip "JRuby doesn't allow recovery from SystemStackErrors" if is_jruby
+
+      class RecursiveHashKey
+        def to_s
+          to_s
+        end
+      end
+
+      key = RecursiveHashKey.new
+
+      a = {}
+      a[key] = 1
+
+      expect(subject.clean_object(a)).to eq({ "[RECURSION]" => "[FILTERED]" })
+    end
+
+    it "cleans up hashes when a nested key infinitely recurse in to_s" do
+      skip "JRuby doesn't allow recovery from SystemStackErrors" if is_jruby
+
+      class RecursiveHashKey
+        def to_s
+          to_s
+        end
+      end
+
+      key = RecursiveHashKey.new
+
+      a = {}
+      a[:b] = {}
+      a[:b][key] = 1
+
+      expected = { :b => { "[RECURSION]" => "[FILTERED]" } }
+
+      expect(subject.clean_object(a)).to eq(expected)
+    end
+
+    it "cleans up hashes when keys raise in to_s" do
+      class RaisingHashKey
+        def to_s
+          raise "hey!"
+        end
+      end
+
+      key = RaisingHashKey.new
+
+      a = {}
+      a[key] = 1
+
+      expect(subject.clean_object(a)).to eq({ "[RAISED]" => "[FILTERED]" })
+    end
+
+    it "cleans up hashes when nested keys raise in to_s" do
+      class RaisingHashKey
+        def to_s
+          raise "hey!"
+        end
+      end
+
+      key = RaisingHashKey.new
+
+      a = {}
+      a[:b] = {}
+      a[:b][key] = 1
+
+      expected = { :b => { "[RAISED]" => "[FILTERED]" } }
+
+      expect(subject.clean_object(a)).to eq(expected)
+    end
+
     it "cleans up recursive arrays" do
       a = []
       a << a
@@ -48,6 +120,20 @@ class Macaron; end
       expect(subject.clean_object(a)).to eq('[OBJECT]')
     end
 
+    it "cleans custom objects when they infinitely recurse" do
+      skip "JRuby doesn't allow recovery from SystemStackErrors" if is_jruby
+
+      class RecursiveObject
+        def to_s
+          to_s
+        end
+      end
+
+      object = RecursiveObject.new
+
+      expect(subject.clean_object(object)).to eq("[RECURSION]")
+    end
+
     it "cleans up binary strings properly" do
       if RUBY_VERSION > "1.9"
         obj = "Andr\xc7\xff"

spec/helper_spec.rb

@@ -50,6 +50,61 @@ def to_s
         value = Bugsnag::Helpers.trim_if_needed([1, 3, StringRaiser.new])
         expect(value[2]).to eq "[RAISED]"
       end
+
+      it "replaces hash key with '[RAISED]'" do
+        a = {}
+        a[StringRaiser.new] = 1
+
+        value = Bugsnag::Helpers.trim_if_needed(a)
+        expect(value).to eq({ "[RAISED]" => "[FILTERED]" })
+      end
+
+      it "uses a single '[RAISED]'key when multiple keys raise" do
+        a = {}
+        a[StringRaiser.new] = 1
+        a[StringRaiser.new] = 2
+
+        value = Bugsnag::Helpers.trim_if_needed(a)
+        expect(value).to eq({ "[RAISED]" => "[FILTERED]" })
+      end
+    end
+
+    context "an object will infinitely recurse if `to_s` is called" do
+      is_jruby = defined?(RUBY_ENGINE) && RUBY_ENGINE == 'jruby'
+
+      class StringRecurser
+        def to_s
+          to_s
+        end
+      end
+
+      it "uses the string '[RECURSION]' instead" do
+        skip "JRuby doesn't allow recovery from SystemStackErrors" if is_jruby
+
+        value = Bugsnag::Helpers.trim_if_needed([1, 3, StringRecurser.new])
+        expect(value[2]).to eq "[RECURSION]"
+      end
+
+      it "replaces hash key with '[RECURSION]'" do
+        skip "JRuby doesn't allow recovery from SystemStackErrors" if is_jruby
+
+        a = {}
+        a[StringRecurser.new] = 1
+
+        value = Bugsnag::Helpers.trim_if_needed(a)
+        expect(value).to eq({ "[RECURSION]" => "[FILTERED]" })
+      end
+
+      it "uses a single '[RECURSION]'key when multiple keys recurse" do
+        skip "JRuby doesn't allow recovery from SystemStackErrors" if is_jruby
+
+        a = {}
+        a[StringRecurser.new] = 1
+        a[StringRecurser.new] = 2
+
+        value = Bugsnag::Helpers.trim_if_needed(a)
+        expect(value).to eq({ "[RECURSION]" => "[FILTERED]" })
+      end
     end
 
     context "payload length is less than allowed" do