diff --git a/.github/actions/check/action.yml b/.github/actions/check/action.yml index e7455238..edd54d69 100644 --- a/.github/actions/check/action.yml +++ b/.github/actions/check/action.yml @@ -23,7 +23,7 @@ runs: - name: Run RuboCop shell: bash - run: bundle exec rubocop --parallel + run: bundle exec rubocop - name: Build contract tests if: ${{ inputs.flaky != 'true' }} diff --git a/lib/ldclient-rb/context.rb b/lib/ldclient-rb/context.rb index 081d61a2..da8e357c 100644 --- a/lib/ldclient-rb/context.rb +++ b/lib/ldclient-rb/context.rb @@ -230,9 +230,9 @@ def get_value_for_reference(reference) name = reference.component(i) return nil unless value.is_a?(Hash) - return nil unless value.has_key?(name) - value = value[name] + found, value = LaunchDarkly::Impl::Context.fetch_attribute(value, name) + return nil unless found end value @@ -418,7 +418,10 @@ def to_h when :anonymous @anonymous else - @attributes&.fetch(name, nil) + return nil if @attributes.nil? + + _, value = LaunchDarkly::Impl::Context.fetch_attribute(@attributes, name) + value end end diff --git a/lib/ldclient-rb/impl/context.rb b/lib/ldclient-rb/impl/context.rb index 8ff6820b..d66a84df 100644 --- a/lib/ldclient-rb/impl/context.rb +++ b/lib/ldclient-rb/impl/context.rb @@ -78,6 +78,42 @@ def self.validate_anonymous(anonymous, allow_nil) ERR_ANONYMOUS_NON_BOOLEAN end + # + # Attribute reference components are always symbols, but application data + # can use string keys. For example, JSON.parse makes string keys by + # default. The two forms name the same JSON property, so this method + # compares them as equal. + # + # @param component [Symbol] + # @param key [any] + # @return [Boolean] + # + def self.same_attribute_name?(component, key) + component == key || component.to_s == key.to_s + end + + # + # Read an attribute out of a hash by name. The hash can use symbol or + # string keys, and the name can be either form. An exact match wins, then + # the other form. A symbol therefore takes precedence when the hash holds + # both forms of one name. + # + # The first element of the returned array is true if the hash has the + # attribute. The second element is the value, or nil if there is none. + # + # @param hash [Hash] + # @param name [Symbol, String] + # @return [Array(Boolean, any)] + # + def self.fetch_attribute(hash, name) + return true, hash[name] if hash.has_key?(name) + + alternate = name.is_a?(Symbol) ? name.to_s : name.to_sym + return true, hash[alternate] if hash.has_key?(alternate) + + [false, nil] + end + # # @param kind [String] # @param key [String] diff --git a/lib/ldclient-rb/impl/context_filter.rb b/lib/ldclient-rb/impl/context_filter.rb index 6fe8d68c..e6ffa84c 100644 --- a/lib/ldclient-rb/impl/context_filter.rb +++ b/lib/ldclient-rb/impl/context_filter.rb @@ -84,7 +84,10 @@ def filter_redact_anonymous(context) end end - filtered[:_meta] = {redactedAttributes: redacted} unless redacted.empty? + # A hash can hold both the string and the symbol form of one name. Each + # form redacts a value, but the reference names one attribute, so report + # it once. + filtered[:_meta] = {redactedAttributes: redacted.uniq} unless redacted.empty? filtered end @@ -137,11 +140,11 @@ def filter_redact_anonymous(context) next unless private_attribute.depth == (current_path.count + 1) component = private_attribute.component(current_path.count) - next unless component == k + next unless LaunchDarkly::Impl::Context.same_attribute_name?(component, k) match = true (0...current_path.count).each do |i| - unless private_attribute.component(i) == current_path[i] + unless LaunchDarkly::Impl::Context.same_attribute_name?(private_attribute.component(i), current_path[i]) match = false break end diff --git a/spec/context_spec.rb b/spec/context_spec.rb index 44be8e4a..35ae5d14 100644 --- a/spec/context_spec.rb +++ b/spec/context_spec.rb @@ -391,6 +391,38 @@ module LaunchDarkly expect(org_context.get_value_for_reference(ref)).to matcher end end + + it "with complex attributes that have string keys" do + address = JSON.parse('{"city":"Oakland","state":"CA","zip":94612}') + tags = ["LaunchDarkly", "Feature Flags"] + nested = JSON.parse('{"upper":{"middle":{"name":"Middle Level","inner":{"levels":[0,1,2]}},"name":"Upper Level"}}') + + org_context = subject.create({ key: 'ld', kind: 'org', name: 'LaunchDarkly', anonymous: true, address: address, tags: tags, nested: nested }) + + [ + ['/address', eq(address)], + ['/address/city', eq('Oakland')], + + ['/tags', eq(tags)], + + ['/nested/upper/name', eq('Upper Level')], + ['/nested/upper/middle/name', eq('Middle Level')], + ['/nested/upper/middle/inner/levels', eq([0, 1, 2])], + ].each do |(reference, matcher)| + ref = Reference.create(reference) + expect(org_context.get_value_for_reference(ref)).to matcher + end + end + + it "with a string top level attribute name" do + address = { city: "Oakland" } + + org_context = subject.create({ :key => 'ld', :kind => 'org', "address" => address }) + + expect(org_context.get_value_for_reference(Reference.create('/address'))).to eq(address) + expect(org_context.get_value_for_reference(Reference.create('/address/city'))).to eq('Oakland') + expect(org_context.get_value("address")).to eq(address) + end end end diff --git a/spec/impl/context_filter_spec.rb b/spec/impl/context_filter_spec.rb index 0c1e205a..e6018288 100644 --- a/spec/impl/context_filter_spec.rb +++ b/spec/impl/context_filter_spec.rb @@ -52,6 +52,89 @@ module Impl expect(filtered["user"][:_meta][:redactedAttributes]).to eq([:email]) expect(filtered["org"]).to eq({ key: "org-key", email: "email" }) end + + describe "with string keyed attribute data" do + it "redacts a nested private attribute" do + filter = ContextFilter.new(false, ["/profile/email"]) + context = LDContext.create({ kind: "user", key: "user-key", profile: JSON.parse('{"email":"private@example.test","plan":"pro"}') }) + + filtered = filter.filter(context) + + expect(filtered[:profile]).to eq({ "plan" => "pro" }) + expect(filtered[:_meta][:redactedAttributes]).to eq([:"/profile/email"]) + end + + it "redacts a nested per-context private attribute" do + filter = ContextFilter.new(false, []) + context = LDContext.create({ + kind: "user", + key: "user-key", + account: JSON.parse('{"apiToken":"private-token","region":"test-region"}'), + _meta: { privateAttributes: ["/account/apiToken"] }, + }) + + filtered = filter.filter(context) + + expect(filtered[:account]).to eq({ "region" => "test-region" }) + expect(filtered[:_meta][:redactedAttributes]).to eq([:"/account/apiToken"]) + end + + it "produces the same event payload as symbol keyed data" do + filter = ContextFilter.new(false, ["/profile/email"]) + json = '{"email":"private@example.test","plan":"pro"}' + string_keyed = LDContext.create({ kind: "user", key: "user-key", profile: JSON.parse(json) }) + symbol_keyed = LDContext.create({ kind: "user", key: "user-key", profile: JSON.parse(json, symbolize_names: true) }) + + # The two hashes keep the key type they were given, so compare the + # payloads the way the event sender sees them. A string key and a + # symbol key serialize to the same JSON property name. + expect(JSON.generate(filter.filter(string_keyed))).to eq(JSON.generate(filter.filter(symbol_keyed))) + end + + it "redacts a private attribute nested three levels deep" do + filter = ContextFilter.new(false, ["/a/b/c"]) + context = LDContext.create({ kind: "user", key: "user-key", a: JSON.parse('{"b":{"c":"private","d":"keep"}}') }) + + filtered = filter.filter(context) + + expect(filtered[:a]).to eq({ "b" => { "d" => "keep" } }) + expect(filtered[:_meta][:redactedAttributes]).to eq([:"/a/b/c"]) + end + + it "redacts both the string and the symbol form of one name, and reports the reference once" do + filter = ContextFilter.new(false, ["/profile/email"]) + context = LDContext.create({ + kind: "user", + key: "user-key", + profile: { :email => "symbol-private", "email" => "string-private", "other" => "keep" }, + }) + + filtered = filter.filter(context) + + expect(filtered[:profile]).to eq({ "other" => "keep" }) + expect(filtered[:_meta][:redactedAttributes]).to eq([:"/profile/email"]) + end + + it "redacts a nested private attribute under a string top level key" do + filter = ContextFilter.new(false, ["/profile/email"]) + context = LDContext.create({ :kind => "user", :key => "user-key", "profile" => JSON.parse('{"email":"private@example.test","plan":"pro"}') }) + + filtered = filter.filter(context) + + expect(filtered["profile"]).to eq({ "plan" => "pro" }) + expect(filtered[:_meta][:redactedAttributes]).to eq([:"/profile/email"]) + end + + it "redacts an escaped nested private attribute name" do + filter = ContextFilter.new(false, ["/profile/a~1b"]) + context = LDContext.create({ kind: "user", key: "user-key", profile: JSON.parse('{"a/b":"private","keep":"keep"}') }) + + filtered = filter.filter(context) + + expect(filtered[:profile]).to eq({ "keep" => "keep" }) + expect(filtered[:_meta][:redactedAttributes]).to eq([:"/profile/a~1b"]) + end + end end end end diff --git a/spec/impl/context_spec.rb b/spec/impl/context_spec.rb index dd798c25..5f2995bf 100644 --- a/spec/impl/context_spec.rb +++ b/spec/impl/context_spec.rb @@ -30,6 +30,44 @@ module Impl expect(subject.validate_key(input)).to eq(expected) end end + + it "compares attribute names in either direction" do + [ + [:email, "email"], + ["email", :email], + [:email, :email], + ["email", "email"], + ].each do |(component, key)| + expect(subject.same_attribute_name?(component, key)).to be true + end + + expect(subject.same_attribute_name?(:email, "other")).to be false + end + + it "fetches an attribute for either key type by either name type" do + [ + [{ email: "value" }, :email], + [{ email: "value" }, "email"], + [{ "email" => "value" }, :email], + [{ "email" => "value" }, "email"], + ].each do |(hash, name)| + expect(subject.fetch_attribute(hash, name)).to eq([true, "value"]) + end + + expect(subject.fetch_attribute({ email: "value" }, :other)).to eq([false, nil]) + end + + it "prefers an exact match when a hash holds both forms of a name" do + hash = { :email => "symbol", "email" => "string" } + + expect(subject.fetch_attribute(hash, :email)).to eq([true, "symbol"]) + expect(subject.fetch_attribute(hash, "email")).to eq([true, "string"]) + end + + it "reports a missing attribute as absent rather than nil valued" do + expect(subject.fetch_attribute({ email: nil }, :email)).to eq([true, nil]) + expect(subject.fetch_attribute({}, :email)).to eq([false, nil]) + end end end end