Uh oh!
There was an error while loading. Please reload this page.
fix: Stop per-context private attributes from accumulating between contexts - #416
Merged
Merged
Conversation
Co-Authored-By: rlamb@launchdarkly.com <4955475+kinyoklion@users.noreply.github.com>
Contributor
🤖 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:
|
Contributor
@cursor review |
kinyoklion
marked this pull request as ready for review
August 17, 2026 17:56
keelerm84
approved these changes
Aug 18, 2026
Uh oh!
There was an error while loading. Please reload this page.
kinyoklion added a commit
to launchdarkly/sdk-test-harness
that referenced
this pull request
Aug 19, 2026
…contexts (#430) Adds contract coverage for private attributes declared in a context's `_meta.privateAttributes` leaking into an SDK's *globally configured* private attribute list, so they get applied to unrelated contexts. - Port of #429 to the `v2` line. Nearly every SDK's CI pulls the harness from `v2` (ruby, php, erlang, rust, haskell, java, dotnet, ios, android, flutter, roku, node-client), so without this the regression below is untested for those SDKs. - 3 new subtests under `events/context properties`; runs for server-side, client-side and PHP suites. No existing test or expectation changed. - Reproduces [ruby-server-sdk#416](launchdarkly/ruby-server-sdk#416): `ContextFilter` did `@private_attributes.concat(context.private_attributes)`, mutating the configured list, so a context that declared nothing private still had the *previous* context's private attributes redacted. **Requirements** - [x] I have added test coverage for new or changed functionality - [x] I have followed the repository's [pull request submission guidelines](../blob/master/CONTRIBUTING.md#submitting-pull-requests) - [ ] I have validated my changes against all supported platform versions — validation across SDKs in progress, results posted as a comment **Related issues** - launchdarkly/ruby-server-sdk#416 — the bug this covers - #429 — the same change on the `v3` line <details> <summary>Implementation details</summary> **Why the existing tests could not catch it** `makeEventContextTestParams` creates a *new client* per parameter, so the `ContextFilter` is always fresh, and every context within one parameter comes from a single factory with identical `_meta.privateAttributes`. A leaked private attribute is therefore always an already-expected private attribute. The existing multi-kind fixtures don't catch it either: they either set no per-context privates, or set them only on the kind that is filtered last, and the leaked names don't exist as attributes on the other kind. **What the new tests do** `eventContextPrivateAttributeScoping` uses **one client** for multiple identify events, and each context carries the same three attributes (`selfPrivate`, `globallyPrivate`, `visible`) while differing only in what it declares private: 1. `private attributes of one context are not applied to later contexts` — identify a context declaring `selfPrivate` private, then identify a different context that declares nothing private, and assert `selfPrivate` is still visible on the second one. 2. + 3. `private attributes of one kind are not applied to other kinds of the same context (declared by org / by user)` — a multi-kind context where only one kind declares `selfPrivate` private; the other kind must keep it. Both orderings are covered because SDKs filter the individual contexts in an arbitrary order — with the Ruby bug present, only the `org` variant fails (Ruby filters `org` first), so a single-ordering test would catch this only half the time. `globallyPrivate` is configured via `GlobalPrivateAttributes` and asserted redacted in every expectation, so an SDK cannot pass by throwing away its configured private attributes along with the per-context ones. **Verification** Negative control against ruby-server-sdk with the #416 fix locally reverted to `.concat(...)`: subtests 1 and 2 fail with the second context wrongly reporting `redactedAttributes: ["selfPrivate","globallyPrivate"]`; all three pass with the fix in place. </details> Link to Devin session: https://app.devin.ai/sessions/6e3076285f2849919b966a4f801075ca Requested by: @kinyoklion <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Overview** > Adds **three new subtests** under `events/context properties` via `eventContextPrivateAttributeScoping`, hooked from `EventContexts`. Existing expectations are unchanged. > > The harness reuses **one SDK client** for multiple identify events so failures like mutating the global private-attribute list (e.g. ruby-server-sdk#416) are detectable—prior cases mostly created a fresh client per scenario. > > **Coverage:** (1) a context that marks `selfPrivate` private must not cause a later context with no per-context privates to redact `selfPrivate`; (2) for multi-kind contexts, privates declared on `org` or `user` must not redact the same attribute on the other kind—both orderings are tested. > > Every assertion still expects **`globallyPrivate`** (from `GlobalPrivateAttributes`) to be redacted so SDKs cannot pass by dropping all configured privates. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 3fa29c4. 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 <devin@launchdarkly.com>
kinyoklion added a commit
to launchdarkly/sdk-test-harness
that referenced
this pull request
Aug 19, 2026
…contexts (#429) Adds `events/context properties` coverage for private attributes declared in a context's `_meta.privateAttributes` leaking onto other contexts — the Ruby SDK bug fixed in launchdarkly/ruby-server-sdk#416, which the existing suite could not detect. - New subtest: two identify events on one client, the first context declaring `selfPrivate` private, the second not — the second must not have it redacted. - New subtests (both kind orderings): one multi-kind context where only one kind declares `selfPrivate` private — the other kind must not have it redacted. - A globally configured private attribute is expected redacted in every assertion, so an SDK cannot pass by dropping its configured private attributes along with the per-context ones. - No new capability gate: this is required behavior for every SDK, so all SDKs run it. **Requirements** - [x] I have added test coverage for new or changed functionality - [x] I have followed the repository's [pull request submission guidelines](../blob/master/CONTRIBUTING.md#submitting-pull-requests) - [ ] I have validated my changes against all supported platform versions <details> <summary>Implementation details</summary> **Why existing tests miss it** `makeEventContextTestParams` builds one client per test param, and every context that param then generates comes from a single `data.ContextFactory` with identical `_meta.privateAttributes`. An SDK that merges per-context private attributes into its long-lived configured list (rather than a per-context copy) therefore leaks only attributes that are already expected to be redacted. The multi-kind factories are also blind to it: they either declare no per-context privates, or declare them only on the kind filtered last (`data.NewContextFactoriesForExercisingAllAttributes` gives `org` just `name` while `other` carries `Private("a","c")`), and the leaked names do not exist on the other kind. **What the new tests do** `CommonEventTests.eventContextPrivateAttributeScoping` is called at the end of `EventContexts`, so it runs for server-side, client-side, and PHP SDKs. Each context carries `selfPrivate`, `globallyPrivate`, and `visible`; `globalPrivateAttributes` is configured as `["globallyPrivate"]`. - Sequential case: identify `selfPrivate`-declaring context (expect `selfPrivate` + `globallyPrivate` redacted), then identify a context that declares nothing private (expect only `globallyPrivate` redacted). Pre-#416 Ruby redacts `selfPrivate` from the second context too. - Multi-kind case: `org` + `user` in one context, only one kind declaring `selfPrivate` private; run once per kind so the leak is caught regardless of the order an SDK filters individual contexts in. Both cases still assert `globallyPrivate` is redacted, which is what distinguishes a correct fix (copy the list) from a wrong one (stop applying configured privates). **Testing** `make build`, `make lint` (0 issues), and `make test` pass. SDK-level validation is in progress: the new subtests are being run against SDK contract test services, including the Ruby SDK before and after #416. **Alternatives considered** Adding more `eventContextTestParams` entries — rejected, the leak is only observable across two contexts with *different* private attributes within one client, which that table cannot express. </details> Link to Devin session: https://app.devin.ai/sessions/6e3076285f2849919b966a4f801075ca Requested by: @kinyoklion <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Overview** > Adds **`eventContextPrivateAttributeScoping`** at the end of **`EventContexts`**, so server-side, client-side, and PHP SDKs all run it. > > The new coverage catches SDKs that merge **`_meta.privateAttributes`** into a long-lived global list instead of a per-context copy (e.g. Ruby server SDK before #416). A **sequential identify** case sends one context that marks **`selfPrivate`** private, then another that does not—only **`globallyPrivate`** (from **`globalPrivateAttributes`**) must be redacted on the second event. **Multi-kind** subtests run for both **`org`** and **`user`** as the kind that declares **`selfPrivate`**, so redaction cannot leak across kinds when filtering one kind at a time. > > Every assertion still expects **`globallyPrivate`** redacted, so passing by dropping all configured private attributes along with per-context ones fails the suite. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 62615b4. 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 <devin@launchdarkly.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Requirements
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_contextcombined the globally configured private attributes with the context's own_meta.privateAttributesusingArray#concat, which mutates@private_attributesin place. Every context filtered by aContextFiltertherefore 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, [])):The fix builds a new array (
@private_attributes + context.private_attributes) instead of mutating the configured list. Event processors reuse a singleContextFilterfor 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
Note
Overview
Fixes a privacy bug in
ContextFilter#filter_single_context: merging global private attributes with each context’s_meta.privateAttributesusedArray#concat, which mutated the filter’s@private_attributes. BecauseEventOutputFormatterkeeps oneContextFilterfor 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_attributesso the configured list is never mutated. New specs cover sequential filtering and multi-kind isolation.Reviewed by Cursor Bugbot for commit 878e2e9. Bugbot is set up for automated code reviews on this repo. Configure here.