fix: Escape attribute names reported in redactedAttributes - #415
fix: Escape attribute names reported in redactedAttributes#415kinyoklion wants to merge 2 commits into
Conversation
Co-Authored-By: rlamb@launchdarkly.com <4955475+kinyoklion@users.noreply.github.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
@cursor review |
…ntexts (#416) **Requirements** - [x] I have added test coverage for new or changed functionality - [x] I have followed the repository's [pull request submission guidelines](../blob/main/CONTRIBUTING.md#submitting-pull-requests) - [ ] I have validated my changes against all supported platform versions **Related issues** Found while investigating the contract test failures fixed in #415. Not covered by the current contract test suite. **Describe the solution you've provided** `ContextFilter#filter_single_context` combined the globally configured private attributes with the context's own `_meta.privateAttributes` using `Array#concat`, which mutates `@private_attributes` in place. Every context filtered by a `ContextFilter` therefore permanently added its private attributes to the filter's configuration, so a context's private attributes were applied to all contexts filtered afterwards — including the other kinds of the same multi-kind context. Before (single `ContextFilter.new(false, [])`): ```ruby filter.filter(user_with_private_email) # {..., _meta: {redactedAttributes: [:email]}} filter.filter(other_user) # {..., _meta: {redactedAttributes: [:email]}} <- email was not private here ``` The fix builds a new array (`@private_attributes + context.private_attributes`) instead of mutating the configured list. Event processors reuse a single `ContextFilter` for the lifetime of the client, so the leak was cumulative across all events. **Describe alternatives you've considered** Constructing the filter per event — unnecessary allocation churn, and the mutation is the actual defect. **Additional context** Full contract test suite against harness v3.2.0-alpha.6 passes with this change (combined with #415): 4723 total, 14 skipped, all ran passed. Link to Devin session: https://app.devin.ai/sessions/316afaccd2604f8d802a72c9080f6921 Requested by: @kinyoklion <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Overview** > **Fixes a privacy bug in `ContextFilter#filter_single_context`:** merging global private attributes with each context’s `_meta.privateAttributes` used `Array#concat`, which mutated the filter’s `@private_attributes`. Because `EventOutputFormatter` keeps one `ContextFilter` for the client lifetime, later contexts (and other kinds in the same multi-kind context) could incorrectly redact attributes that were only private on a previous context. > > The change uses `@private_attributes + context.private_attributes` so the configured list is never mutated. New specs cover sequential filtering and multi-kind isolation. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 878e2e9. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
| private def check_whole_attribute_private(attribute, private_attributes, redacted, redact_all) | ||
| if @all_attributes_private || redact_all | ||
| redacted << attribute | ||
| redacted << redaction_name(attribute) |
There was a problem hiding this comment.
I would expect this to have been done earlier in the process than here? Is any other functionality impacted by this?
There was a problem hiding this comment.
Moved it earlier: filter_single_context now builds Reference.create_literal(attribute) where the attribute names are enumerated, and check_whole_attribute_private takes that reference (matching on component(0), reporting raw_path).
Nothing else is impacted. ContextFilter is the only consumer of get_custom_attribute_names, and it's the only place that turns a plain attribute name into a reported reference — nested redaction already reported private_attribute.raw_path, and LDContext#to_h/private_attributes already store references. The other create_literal callers (clause attributes, bucket_by) are unrelated to event serialization.
There was a problem hiding this comment.
I just want an answer, but not further changes. Was check_whole_attribute_private already working fine with strings, did it know internally that it needed to handle this nuance.
Does the SDK access fields by attribute reference? Is it accessing those fields correctly regardless of reference format? Does it still support literals via a user type, or does it only support contexts where the input should be references?
There was a problem hiding this comment.
Answers only, no further changes:
Was check_whole_attribute_private already fine? Yes, for matching. It compares private_attribute.component(0) (a parsed reference component, already unescaped) against the raw attribute name, so a configured private attribute of /~1ssn correctly matched the literal attribute named /ssn. The only defect was reporting: the raw name was pushed into redactedAttributes instead of its reference form. So the change is output-only — nothing about the match logic needed the nuance, and the bug was invisible unless an attribute name contained / or ~.
Does the SDK access fields by attribute reference? Yes — LDContext#get_value_for_reference is the single accessor and it walks Reference components, so lookups are correct regardless of reference format. get_value(name) is just get_value_for_reference(Reference.create_literal(name)), which is why ContextFilter reading /ssn as a literal always worked.
Literals vs references at the input boundary: literal handling is retained where the input historically wasn't a reference — Clause#attribute and bucket_by use create_literal when contextKind is absent (old user-schema flag rules) and Reference.create when it's present. Everything context-facing treats input as references: Config#private_attributes and _meta.privateAttributes are parsed with Reference.create, and to_h emits them as raw_path. redactedAttributes is the same wire field family, hence the escaping here.
Co-Authored-By: rlamb@launchdarkly.com <4955475+kinyoklion@users.noreply.github.com>
| private_attributes.each do |private_attribute| | ||
| if private_attribute.component(0) == attribute && private_attribute.depth == 1 | ||
| redacted << attribute | ||
| if private_attribute.component(0) == attribute.component(0) && private_attribute.depth == 1 |
There was a problem hiding this comment.
How does this component check work in the case of a path? /address/street for example? Just answer.
There was a problem hiding this comment.
It never fires for a path. attribute here is always a top-level attribute name wrapped with create_literal, so it's depth 1, and the loop also requires private_attribute.depth == 1. A private attribute of /address/street has depth 2, so this check skips it and redact_json_value handles it by walking address → street and reporting private_attribute.raw_path (/address/street) — unchanged by this PR.
component(0) on both sides is just "the unescaped top-level name", so the two depth-1 cases still line up:
- private
address(or/address) vs. attribute namedaddress→ bothcomponent(0) == :address→ match. - private
/~1address~1streetvs. a top-level attribute literally named/address/street→ the private reference parses to depth 1 withcomponent(0) == :"/address/street", andcreate_literal("/address/street")gives the samecomponent(0)withraw_path/~1address~1street→ match, reported escaped.
Comparing component(0) rather than the raw name is what makes the second case work; the old code compared component(0) against the raw name, which happened to work for matching but reported the unescaped name.
There was a problem hiding this comment.
It feels strange to me that there are two paths. What is the root reason for that asymmetry? The root storage being different than the storage within an attribute?
There was a problem hiding this comment.
The asymmetry predates this PR and comes from the two paths starting from different things, in opposite directions:
- Top level (
check_whole_attribute_private) iterates over attribute names the context holds and asks "is this name private?". Names are arbitrary strings that are not references (/ssnis a legal attribute name), so a reference has to be constructed from the name to report it — that construction is the escaping this PR adds. It also has redaction triggers that only exist at level 0:allAttributesPrivateand anonymous redaction, plus the built-in/non-addressable keys (key,kind,anonymous,_meta) that must never be redacted andname, which is special-cased. - Inside an attribute (
redact_json_value) iterates over private attribute references against arbitrary JSON. It already holds a reference that matched, so it reportsraw_pathdirectly — nothing to construct — and it can't drop the value outright; it has to rebuild the object minus redacted keys.
So it's less "root vs. nested storage" and more: level 0 is a name-keyed context with special-cased keys and blanket-redaction rules, level 1+ is plain JSON addressed only by reference components. Unifying them would mean treating the context as a JSON object one level up, which the special keys at level 0 make awkward — and the spec's escaping rule for names only exists because of that level-0 name-to-reference conversion.
There was a problem hiding this comment.
Sure sounds like roost vs nested. Where root is a name-keyed context with special-cased keys, and nested items are JSON.
There was a problem hiding this comment.
Agreed — root vs. nested is the right framing, and my wording was splitting hairs. Root is a name-keyed context with special-cased keys (hence the name→reference escaping this PR adds); nested is plain JSON already addressed by references.
1 participant
Requirements
Related issues
Surfaced by contract test harness v3.2.0-alpha.6 (the pin bump in #414). These 6 subtests failed against the newer harness:
events/context properties/single-kind, allAttributesPrivate, slash-prefixed attribute name(debug, identify, index-from-evaluation, index-from-custom-event)events/feature events/single-kind anonymous context redacts all attributes/type: anyevents/feature events/multi-kind with anonymous context redacts attributes appropriately/type: anyDescribe the solution you've provided
_meta.redactedAttributesin event payloads is a list of attribute references, not raw attribute names. When a context has an attribute whose name begins with/(e.g./ssn), the SDK emitted the raw name, which a consumer parses as a path expression pointing at a nested property rather than the top-level attribute:ContextFilter#check_whole_attribute_privatenow converts the attribute name to a reference withReference.create_literalbefore adding it to the redacted list, which escapes/and~for slash-prefixed names and leaves all other names unchanged (namestaysname,a/b~cstaysa/b~csince it is already a literal reference). This covers both theallAttributesPrivate/configured-private paths and the anonymous-context redaction path, which is why one change fixes all 6 subtests.Verified locally: contract test service against released harness v3.2.0-alpha.6, full suite — 4723 total, 14 skipped, all ran passed.
Describe alternatives you've considered
Escaping only in the
all_attributes_privatebranch — rejected, the same escaping is required wherever a whole attribute is redacted.Additional context
Nested redactions (
redact_json_value) already reportedReference#raw_path, so they were unaffected.Link to Devin session: https://app.devin.ai/sessions/316afaccd2604f8d802a72c9080f6921
Requested by: @kinyoklion
Note
Overview
Fixes
_meta.redactedAttributesin event payloads so entries are escaped attribute references, not raw names. Attributes whose names start with/(e.g./ssn) were emitted as/ssn, which consumers parse as a nested path instead of a top-level literal.ContextFilternow passesReference.create_literalintocheck_whole_attribute_privatefornameand custom attributes, and recordsattribute.raw_path.to_symin the redacted list. That aligns whole-attribute redaction withredact_json_value, which already usedraw_path.The same path covers all-attributes-private, configured private attributes, and anonymous context redaction. Tests assert escaped values such as
"/~1ssn"for slash-prefixed names.Reviewed by Cursor Bugbot for commit 5814e95. Bugbot is set up for automated code reviews on this repo. Configure here.