diff --git a/lib/ldclient-rb/events.rb b/lib/ldclient-rb/events.rb index b5df9711..f056a9aa 100644 --- a/lib/ldclient-rb/events.rb +++ b/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. diff --git a/lib/ldclient-rb/impl/context_filter.rb b/lib/ldclient-rb/impl/context_filter.rb index 6fe8d68c..30c6ecdb 100644 --- a/lib/ldclient-rb/impl/context_filter.rb +++ b/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] + # @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] # @param redacted [Array] + # @param visited [Array] # @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 diff --git a/spec/impl/context_filter_spec.rb b/spec/impl/context_filter_spec.rb index 0c1e205a..c1915482 100644 --- a/spec/impl/context_filter_spec.rb +++ b/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