stuff

Author: sl0thentr0py

CHANGELOG.md

@@ -64,10 +64,6 @@ We recommend migrating to `data_collection` to match the behavior you want event
 - Add sensitive key-value collection filter by @sl0thentr0py in [#3025](https://github.com/getsentry/sentry-ruby/pull/3025)
 - Add base DataCollection configuration with defaults and backfill by @sl0thentr0py in [#3022](https://github.com/getsentry/sentry-ruby/pull/3022)
 
-### Bug Fixes 🐛
-
-- Sanitize breadcrumb data and structured-log attribute values to valid UTF-8 where they're filled in, and add a last-resort rescue for `EncodingError`/`JSON::GeneratorError` in `Transport#send_envelope`, to support the `json` gem 3.0, which now raises instead of warning when generating JSON from a String tagged with a non-UTF-8 encoding. Fixes #2462
-
 ## 6.7.0
 
 ### New Features ✨

sentry-ruby/lib/sentry/breadcrumb.rb

@@ -2,9 +2,6 @@
 
 module Sentry
   class Breadcrumb
-    MAX_NESTING = 10
-    DATA_SERIALIZATION_ERROR_MESSAGE = "[data were removed due to serialization issues]"
-
     # @return [String, nil]
     attr_accessor :category
     # @return [Hash, nil]
@@ -37,7 +34,7 @@ def initialize(category: nil, data: nil, message: nil, timestamp: nil, level: ni
     def to_h
       {
         category: @category,
-        data: serialized_data,
+        data: @data,
         level: @level,
         message: @message,
         timestamp: @timestamp,
@@ -51,8 +48,7 @@ def message=(message)
       @message = message && Utils::EncodingHelper.valid_utf_8?(message) ? message.byteslice(0..Event::MAX_MESSAGE_SIZE_IN_BYTES) : ""
     end
 
-    # Sanitizes the breadcrumb's arbitrary, user-supplied data so it doesn't
-    # carry a String with an invalid/non-UTF-8 encoding into JSON generation.
+    # Sanitizes the breadcrumb's arbitrary, user-supplied data encoding.
     # @param data [Hash, nil]
     # @return [void]
     def data=(data)
@@ -64,22 +60,5 @@ def data=(data)
     def level=(level) # needed to meet the Sentry spec
       @level = level == "warn" ? "warning" : level
     end
-
-    private
-
-    def serialized_data
-      begin
-        ::JSON.parse(::JSON.generate(@data, max_nesting: MAX_NESTING))
-      rescue Exception => e
-        Sentry.sdk_logger.debug(LOGGER_PROGNAME) do
-          <<~MSG
-can't serialize breadcrumb data because of error: #{e}
-data: #{@data}
-          MSG
-        end
-
-        { error: DATA_SERIALIZATION_ERROR_MESSAGE }
-      end
-    end
   end
 end

sentry-ruby/lib/sentry/envelope/item.rb

@@ -72,6 +72,8 @@ def serialize
       end
 
       [result, result.bytesize > size_limit]
+    rescue EncodingError, JSON::GeneratorError => e
+      [nil, false, e]
     end
 
     def size_breakdown

sentry-ruby/lib/sentry/transport.rb

@@ -67,28 +67,21 @@ def send_envelope(envelope)
       serialized_items&.each do |item|
         record_lost_event(:send_error, item.data_category, num: item.item_count, num_bytes: item.lost_event_byte_size)
       end
-    rescue EncodingError, JSON::GeneratorError => e
-      # As of json 3.0, `JSON.generate` raises instead of warning when it
-      # encounters a String with an invalid/non-UTF-8 encoding (e.g. a
-      # BINARY-tagged String). We sanitize known data-entry points (e.g.
-      # breadcrumb data, log attributes), but this is a last resort so an
-      # unexpected case can't crash the background worker.
-      log_error("[Transport] Failed to serialize envelope", e, debug: @debug)
-
-      # `serialized_items` may still be nil here if the error was raised
-      # while serializing the envelope itself (rather than while sending
-      # already-serialized data), so fall back to the envelope's own items.
-      (serialized_items || envelope.items).each do |item|
-        record_lost_event(:send_error, item.data_category, num: item.item_count)
-      end
     end
 
     def serialize_envelope(envelope)
       serialized_items = []
       serialized_results = []
 
       envelope.items.each do |item|
-        result, oversized = item.serialize
+        result, oversized, serialization_error = item.serialize
+
+        if serialization_error
+          log_error("[Transport] Failed to serialize envelope item [#{item.type}]", serialization_error, debug: @debug)
+          record_lost_event(:send_error, item.data_category, num: item.item_count)
+
+          next
+        end
 
         if oversized
           log_debug("Envelope item [#{item.type}] is still oversized after size reduction: {#{item.size_breakdown}}")

sentry-ruby/lib/sentry/utils/encoding_helper.rb

@@ -27,25 +27,44 @@ def self.safe_utf_8_string(value)
       # Recursively walks a Hash/Array/String structure and returns a copy
       # with every String forced into valid UTF-8 encoding.
       #
-      # This is needed because `JSON.generate`/`JSON.dump` on json 3.0+ raise
-      # an `Encoding::UndefinedConversionError` (previously just a
-      # deprecation warning on json 2.8+) when they encounter a String
-      # tagged with a non-UTF-8 encoding (e.g. `ASCII-8BIT`/`BINARY`) that
-      # contains bytes invalid for the target encoding. Use this to sanitize
-      # payloads before handing them to the JSON generator.
+      # Circular Hash and Array references are replaced with nil in the
+      # returned copy.
       #
       # @param value [Object]
       # @return [Object]
-      def self.deep_encode_utf_8(value)
+      def self.deep_encode_utf_8(value, seen = {})
         case value
         when String
           encode_to_utf_8(value)
         when Hash
-          value.each_with_object({}) do |(key, val), memo|
-            memo[deep_encode_utf_8(key)] = deep_encode_utf_8(val)
+          return nil if seen.key?(value.object_id)
+
+          seen[value.object_id] = true
+          encoded_value = {}
+
+          begin
+            value.each do |key, val|
+              encoded_key = deep_encode_utf_8(key, seen)
+              encoded_value[encoded_key] = deep_encode_utf_8(val, seen)
+            end
+            encoded_value
+          ensure
+            seen.delete(value.object_id)
           end
         when Array
-          value.map { |val| deep_encode_utf_8(val) }
+          return nil if seen.key?(value.object_id)
+
+          seen[value.object_id] = true
+          encoded_value = []
+
+          begin
+            value.each do |val|
+              encoded_value << deep_encode_utf_8(val, seen)
+            end
+            encoded_value
+          ensure
+            seen.delete(value.object_id)
+          end
         else
           value
         end

sentry-ruby/spec/sentry/breadcrumb_buffer_spec.rb

@@ -23,20 +23,6 @@
     )
   end
 
-  let(:problematic_crumb) do
-    # circular reference
-    a = []
-    b = []
-    a.push(b)
-    b.push(a)
-
-    Sentry::Breadcrumb.new(
-      category: "baz",
-      message: "crumb_3",
-      data: a
-    )
-  end
-
   describe "#record" do
     subject do
       described_class.new(1)
@@ -56,18 +42,15 @@
   end
 
   describe "#to_h" do
-    it "doesn't break because of 1 problematic crumb" do
+    it "serializes breadcrumbs" do
       subject.record(crumb_1)
       subject.record(crumb_2)
-      subject.record(problematic_crumb)
 
       result = subject.to_h[:values]
 
       expect(result[0][:category]).to eq("foo")
-      expect(result[0][:data]).to eq({ "name" => "John", "age" => 25 })
+      expect(result[0][:data]).to eq({ name: "John", age: 25 })
       expect(result[1][:category]).to eq("bar")
-      expect(result[2][:category]).to eq("baz")
-      expect(result[2][:data][:error]).to eq("[data were removed due to serialization issues]")
     end
   end
 end

sentry-ruby/spec/sentry/breadcrumb_spec.rb

@@ -79,40 +79,21 @@
       )
     end
 
-    let(:very_deep_crumb) do
-      data = [[[[[ { a: [{ b: [[{ c: 4 }]] }] }]]]]]
-
-      Sentry::Breadcrumb.new(
-        category: "cow",
-        message: "I cause too much recursion",
-        data: data
-      )
-    end
-
-    it "serializes data correctly" do
+    it "returns the sanitized data" do
       result = crumb.to_h
 
       expect(result[:category]).to eq("foo")
       expect(result[:message]).to eq("crumb")
-      expect(result[:data]).to eq({ "name" => "John", "age" => 25 })
+      expect(result[:data]).to eq({ name: "John", age: 25 })
     end
 
-    it "rescues data serialization issue and ditch the data" do
+    it "handles a circular breadcrumb without recursing forever" do
       result = problematic_crumb.to_h
 
       expect(result[:category]).to eq("baz")
       expect(result[:message]).to eq("I cause issues")
-      expect(result[:data][:error]).to eq("[data were removed due to serialization issues]")
-      expect(stringio.string).to match(/can't serialize breadcrumb data because of error: nesting of 10 is too deep/)
-    end
-
-    it "rescues data serialization issue for extremely nested data and ditch the data" do
-      result = very_deep_crumb.to_h
-
-      expect(result[:category]).to eq("cow")
-      expect(result[:message]).to eq("I cause too much recursion")
-      expect(result[:data][:error]).to eq("[data were removed due to serialization issues]")
-      expect(stringio.string).to match(/can't serialize breadcrumb data because of error: nesting of 10 is too deep/)
+      expect(result[:data]).to eq([[nil]])
+      expect { JSON.generate(result[:data]) }.not_to raise_error
     end
 
     it "sanitizes non-UTF-8 encoded strings in data at assignment time (json 3.0+ behavior)" do
@@ -128,7 +109,8 @@
       expect(crumb.data[:note].valid_encoding?).to eq(true)
 
       result = crumb.to_h
-      expect(result[:data]).not_to eq({ error: Sentry::Breadcrumb::DATA_SERIALIZATION_ERROR_MESSAGE })
+      expect(result[:data][:note].encoding).to eq(Encoding::UTF_8)
+      expect(result[:data][:note].valid_encoding?).to eq(true)
     end
   end
 end

sentry-ruby/spec/sentry/transport_spec.rb

@@ -499,6 +499,20 @@
       end
     end
 
+    context "when sending raises an encoding error" do
+      let(:event) { client.event_from_exception(ZeroDivisionError.new("divided by 0")) }
+      let(:envelope) { subject.envelope_from_event(event) }
+
+      before do
+        allow(subject).to receive(:send_data).and_raise(EncodingError, "simulated send error")
+      end
+
+      it "does not handle the error as a serialization failure" do
+        expect { subject.send_envelope(envelope) }.to raise_error(EncodingError, "simulated send error")
+        expect(io.string).not_to match(/Failed to serialize envelope/)
+      end
+    end
+
     context "transaction event" do
       let(:transaction) do
         Sentry::Transaction.new(name: "test transaction", op: "rack.request")
@@ -652,27 +666,33 @@
       end
     end
 
-    context "when JSON.generate raises an encoding error (json 3.0+ behavior)" do
-      # json 3.0+ raises Encoding::UndefinedConversionError (a subclass of
-      # EncodingError) instead of just warning when JSON.generate encounters
-      # a String tagged with a non-UTF-8 encoding that contains bytes invalid
-      # for the target encoding. Simulate that here regardless of the json
-      # gem version actually loaded, as a last-resort safety net for cases
-      # not already covered by sanitizing data at the point it's filled in
-      # (e.g. breadcrumb data, log attributes).
-      let(:event) { client.event_from_exception(ZeroDivisionError.new("divided by 0")) }
-      let(:envelope) { subject.envelope_from_event(event) }
+    context "when JSON.generate raises an encoding error for an item (json 3.0+ behavior)" do
+      let(:bad_payload) { { message: "bad payload" } }
+      let(:good_payload) { { message: "good payload" } }
+      let(:envelope) do
+        Sentry::Envelope.new.tap do |new_envelope|
+          new_envelope.add_item({ type: "event" }, bad_payload)
+          new_envelope.add_item({ type: "event" }, good_payload)
+        end
+      end
 
       before do
-        allow(JSON).to receive(:generate).and_raise(EncodingError, "simulated json 3.0 encoding error")
+        allow(JSON).to receive(:generate).and_wrap_original do |original, value|
+          raise EncodingError, "simulated json 3.0 encoding error" if value.equal?(bad_payload)
+
+          original.call(value)
+        end
       end
 
-      it "does not raise, logs the failure, and records a lost event instead of sending" do
-        expect(subject).not_to receive(:send_data)
+      it "skips the failed item, sends the remaining items, and records the loss" do
+        expect(subject).to receive(:send_data) do |data|
+          expect(data).to include("good payload")
+          expect(data).not_to include("bad payload")
+        end
 
         expect { subject.send_envelope(envelope) }.not_to raise_error
 
-        expect(io.string).to match(/Failed to serialize envelope/)
+        expect(io.string).to match(/Failed to serialize envelope item/)
         expect(subject).to have_recorded_lost_event(:send_error, 'error')
       end
     end

sentry-ruby/spec/sentry/utils/encoding_helper_spec.rb

@@ -55,5 +55,15 @@
 
       expect(value[:message].encoding).to eq(Encoding::BINARY)
     end
+
+    it "replaces circular references without recursing forever" do
+      value = []
+      value << value
+
+      result = described_class.deep_encode_utf_8(value)
+
+      expect(result).to eq([nil])
+      expect(value.first).to equal(value)
+    end
   end
 end