fix: Omit non-symbol context attributes and protect against cyclic values

lib/ldclient-rb/events.rb

@@ -487,7 +487,7 @@ class EventOutputFormatter
     SUMMARY_KIND = 'summary'
 
     def initialize(config)
-      @context_filter = LaunchDarkly::Impl::ContextFilter.new(config.all_attributes_private, config.private_attributes)
+      @context_filter = LaunchDarkly::Impl::ContextFilter.new(config.all_attributes_private, config.private_attributes, config.logger)
     end
 
     # Transforms events into the format used for event sending.

lib/ldclient-rb/impl/context_filter.rb

@@ -1,12 +1,17 @@
+require "concurrent/atomics"
+
 module LaunchDarkly
   module Impl
     class ContextFilter
       #
       # @param all_attributes_private [Boolean]
       # @param private_attributes [Array<String>]
+      # @param logger [Logger, nil]
       #
-      def initialize(all_attributes_private, private_attributes)
+      def initialize(all_attributes_private, private_attributes, logger = nil)
         @all_attributes_private = all_attributes_private
+        @logger = logger
+        @non_symbol_name_logged = Concurrent::AtomicBoolean.new(false)
 
         @private_attributes = []
         private_attributes.each do |attribute|
@@ -78,9 +83,14 @@ def filter_redact_anonymous(context)
         end
 
         context.get_custom_attribute_names.each do |attribute|
+          unless attribute.is_a?(Symbol)
+            log_non_symbol_name(attribute)
+            next
+          end
+
           unless check_whole_attribute_private(Reference.create_literal(attribute), private_attributes, redacted, anonymous && redact_anonymous)
             value = context.get_value(attribute)
-            filtered[attribute] = redact_json_value(nil, attribute, value, private_attributes, redacted)
+            filtered[attribute] = redact_json_value(nil, attribute, value, private_attributes, redacted, [])
           end
         end
 
@@ -122,16 +132,34 @@ def filter_redact_anonymous(context)
       # @param value [any]
       # @param private_attributes [Array<Reference>]
       # @param redacted [Array<Symbol>]
+      # @param visited [Array<Hash>]
       # @return [any]
       #
-      private def redact_json_value(parent_path, name, value, private_attributes, redacted)
+      private def redact_json_value(parent_path, name, value, private_attributes, redacted, visited)
         return value unless value.is_a?(Hash)
 
         ret = {}
         current_path = parent_path.clone || []
         current_path << name
 
+        # The chain of hashes from the attribute root down to this value. A
+        # nested value that points back into this chain is a cycle and is
+        # omitted.
+        #
+        # The comparison is by object identity, which keeps the check cheap.
+        # Context creation copies each top-level attribute value, so a cycle
+        # through that value appears once in the output before it is cut.
+        # Private attribute references match the paths that appear in the
+        # output. Cyclic values are not valid context data; this check only
+        # prevents unbounded recursion.
+        branch = visited + [value]
+
         value.each do |k, v|
+          unless k.is_a?(Symbol)
+            log_non_symbol_name(k)
+            next
+          end
+
           was_redacted = false
           private_attributes.each do |private_attribute|
             next unless private_attribute.depth == (current_path.count + 1)
@@ -155,12 +183,34 @@ def filter_redact_anonymous(context)
           end
 
           unless was_redacted
-            ret[k] = redact_json_value(current_path, k, v, private_attributes, redacted)
+            next if v.is_a?(Hash) && branch.any? { |seen| seen.equal?(v) }
+
+            ret[k] = redact_json_value(current_path, k, v, private_attributes, redacted, branch)
           end
         end
 
         ret
       end
+
+      #
+      # Log the first attribute found with a non-symbol name. Later
+      # occurrences are not logged to prevent log spam.
+      #
+      # Attribute references cannot address non-symbol names, so these
+      # attributes cannot be evaluated or redacted. They are omitted from
+      # analytics events.
+      #
+      # @param name [any]
+      #
+      private def log_non_symbol_name(name)
+        return if @logger.nil?
+        return unless @non_symbol_name_logged.make_true
+
+        @logger.error do
+          "[LDClient] Context attributes with non-symbol names cannot be evaluated or redacted, " \
+            "so they are omitted from analytics events (first occurrence: #{name.inspect}). This message is logged once."
+        end
+      end
     end
   end
 end

spec/impl/context_filter_spec.rb

@@ -1,4 +1,5 @@
 require "spec_helper"
+require "capturing_logger"
 
 module LaunchDarkly
   module Impl
@@ -52,6 +53,113 @@ module Impl
         expect(filtered["user"][:_meta][:redactedAttributes]).to eq([:email])
         expect(filtered["org"]).to eq({ key: "org-key", email: "email" })
       end
+
+      it "omits top-level attributes with non-symbol names" do
+        filter = ContextFilter.new(false, [])
+        context = LDContext.create({ kind: "user", key: "user-key", "legacy" => "value", plan: "basic" })
+
+        filtered = filter.filter(context)
+
+        expect(filtered).to eq({ key: "user-key", kind: "user", plan: "basic" })
+      end
+
+      it "omits nested attributes with non-symbol names at any depth" do
+        filter = ContextFilter.new(false, [])
+        context = LDContext.create({ kind: "user", key: "user-key",
+                                     address: { street: "123 Easy St", "city" => "Springfield",
+                                                extra: { "deep" => 1, depth: 2 } } })
+
+        filtered = filter.filter(context)
+
+        expect(filtered[:address]).to eq({ street: "123 Easy St", extra: { depth: 2 } })
+      end
+
+      it "omits the whole subtree under a non-symbol name" do
+        filter = ContextFilter.new(false, [])
+        context = LDContext.create({ kind: "user", key: "user-key",
+                                     profile: { "nested" => { a: 1 }, ok: true } })
+
+        filtered = filter.filter(context)
+
+        expect(filtered[:profile]).to eq({ ok: true })
+      end
+
+      it "does not report attributes with non-symbol names as redacted" do
+        filter = ContextFilter.new(true, [])
+        context = LDContext.create({ kind: "user", key: "user-key", "legacy" => "value", plan: "basic" })
+
+        filtered = filter.filter(context)
+
+        expect(filtered[:_meta][:redactedAttributes]).to eq([:plan])
+      end
+
+      it "redacts private attributes while omitting attributes with non-symbol names" do
+        filter = ContextFilter.new(false, ["/address/zip"])
+        context = LDContext.create({ kind: "user", key: "user-key",
+                                     address: { street: "123 Easy St", "city" => "Springfield", zip: "97475" } })
+
+        filtered = filter.filter(context)
+
+        expect(filtered[:address]).to eq({ street: "123 Easy St" })
+        expect(filtered[:_meta][:redactedAttributes]).to eq([:"/address/zip"])
+      end
+
+      it "logs an error only once when attributes with non-symbol names are omitted" do
+        logger = CapturingLogger.new
+        filter = ContextFilter.new(false, [], logger)
+        context = LDContext.create({ kind: "user", key: "user-key", "legacy" => "value",
+                                     address: { "city" => "Springfield" } })
+
+        filter.filter(context)
+        filter.filter(context)
+
+        expect(logger.output.scan(/non-symbol names/).length).to eq(1)
+        expect(logger.output).to include("ERROR")
+      end
+
+      it "does not log when all attribute names are symbols" do
+        logger = CapturingLogger.new
+        filter = ContextFilter.new(false, [], logger)
+        context = LDContext.create({ kind: "user", key: "user-key", plan: "basic" })
+
+        filter.filter(context)
+
+        expect(logger.output).to eq("")
+      end
+
+      it "omits a nested value that refers to its own ancestor" do
+        address = { street: "123 Easy St" }
+        address[:self] = address
+        filter = ContextFilter.new(false, [])
+        context = LDContext.create({ kind: "user", key: "user-key", address: address })
+
+        filtered = filter.filter(context)
+
+        expect(filtered[:address]).to eq({ street: "123 Easy St", self: { street: "123 Easy St" } })
+      end
+
+      it "matches private references against the paths present in the output for cyclic values" do
+        address = { street: "123 Easy St", city: "Springfield" }
+        address[:self] = address
+        filter = ContextFilter.new(false, ["/address/street"])
+        context = LDContext.create({ kind: "user", key: "user-key", address: address })
+
+        filtered = filter.filter(context)
+
+        expect(filtered[:address]).to eq({ city: "Springfield", self: { street: "123 Easy St", city: "Springfield" } })
+        expect(filtered[:_meta][:redactedAttributes]).to eq([:"/address/street"])
+      end
+
+      it "keeps a value that appears in more than one branch" do
+        shared = { city: "Springfield" }
+        filter = ContextFilter.new(false, [])
+        context = LDContext.create({ kind: "user", key: "user-key",
+                                     addresses: { home: shared, work: shared } })
+
+        filtered = filter.filter(context)
+
+        expect(filtered[:addresses]).to eq({ home: { city: "Springfield" }, work: { city: "Springfield" } })
+      end
     end
   end
 end