fix: Authenticate release asset downloads with GITHUB_TOKEN - #416
Draft
kinyoklion wants to merge 2 commits into
Draft
fix: Authenticate release asset downloads with GITHUB_TOKEN#416kinyoklion wants to merge 2 commits into
kinyoklion wants to merge 2 commits into
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 |
Co-Authored-By: rlamb@launchdarkly.com <4955475+kinyoklion@users.noreply.github.com>
kinyoklion
added a commit
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
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>
kinyoklion
added a commit
that referenced
this pull request
Aug 25, 2026
**Requirements** - [ ] 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) - [x] I have validated my changes against all supported platform versions **Related issues** Port of #415 (merged to `main`) and #416 (`v2`) to `v3`. The contract-tests action fetches `run.sh` from the `v2` branch today, so this port keeps `v3` from regressing when it becomes the branch CI uses. **Describe the solution you've provided** Previously `GITHUB_TOKEN` was only used for the releases-listing request in `resolve_version`, so any run that passed a full version string (as CI usually does) made no authenticated requests at all, and the asset download was always anonymous. The `github.com/.../releases/download/...` path is not a REST endpoint: GitHub publishes no rate limit for it and returns no `x-ratelimit-*` headers, and passing a token there is useless because `curl -L` drops `Authorization` on the cross-host redirect to the signed asset host. This change routes the download through the documented release-assets API when a token is present: - `github_curl` centralizes auth (`Authorization: Bearer`) plus `-sS --retry 5 --retry-delay 2`, replacing the previous `eval`-built command string that interpolated the token into a shell command. - `resolve_asset_url` looks up the asset for the current OS/arch via `GET /releases/tags/<version>` and downloads it from `GET /releases/assets/<id>` with `Accept: application/octet-stream`. That lands in the documented authenticated rate limit bucket instead of an undocumented one. - Without a token — or if the asset lookup fails — the token is cleared and it falls back to the previous anonymous `releases/download` URL, so external consumers are unaffected. Docs referenced: [rate limits for the REST API](https://docs.github.com/en/rest/using-the-rest-api/rate-limits-for-the-rest-api), [release assets endpoints](https://docs.github.com/en/rest/releases/assets). **Describe alternatives you've considered** - Sending the token to the existing `releases/download` URL — verified that this has no effect on the actual byte transfer, since the header is dropped at the redirect. - Retry-only (#411 on `v2`) — rides out transient failures, but leaves the download in the unauthenticated bucket. **How to test it** ```sh export VERSION=v3 PARAMS="-help" GITHUB_TOKEN=<token> sh downloader/run.sh # downloads from .../releases/assets/<id> sh downloader/run.sh # falls back to .../releases/download/... GITHUB_TOKEN=bogus sh downloader/run.sh # prints the 401, then falls back anonymously ``` Also exercised full and partial version strings, the cached-binary path, and an unmatched version. **Additional context** Shell-script-only change used by CI, so no UI screenshots apply. This also brings the retry and error-surfacing behavior of #411 to `v3`, which never received it. The Windows `zip` path is unchanged apart from going through `github_curl`. Link to Devin session: https://app.devin.ai/sessions/98e5024773d947509ad2575ae80bf31b Requested by: @kinyoklion <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Overview** > **CI downloader** (`downloader/run.sh`) now uses `GITHUB_TOKEN` for **release asset downloads**, not only when resolving partial version tags from the releases list. > > A shared **`github_curl`** helper sends **`Authorization: Bearer`** with retries and replaces the old `eval`-built curl that only authenticated the releases list—and did not help downloads when CI passed a full version string. > > When a token is set, **`resolve_asset_url`** fetches the OS/arch archive via the **releases assets REST API** (`Accept: application/octet-stream`), avoiding the public `releases/download` URL where **`curl -L` drops auth on redirect**. If asset lookup fails or no token is provided, behavior falls back to the anonymous download URL (token cleared on fallback so a bad token does not break public downloads). > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 7a13940. 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>
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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
Port of #415 (merged to
main) tov2; #418 is thev3port. Follows #411, which added retries and surfaced curl errors but left the download unauthenticated. This is the branch thelaunchdarkly/gh-actionscontract-tests action actually fetchesrun.shfrom, so this is the one that affects SDK CI.Describe the solution you've provided
Previously
GITHUB_TOKENwas only used for the releases-listing request inresolve_version, so any run that passed a full version string (as CI usually does) made no authenticated requests at all, and the asset download was always anonymous. Thegithub.com/.../releases/download/...path is not a REST endpoint: GitHub publishes no rate limit for it and returns nox-ratelimit-*headers, and passing a token there is useless becausecurl -LdropsAuthorizationon the cross-host redirect to the signed asset host.This change routes the download through the documented release-assets API when a token is present:
github_curlcentralizes auth (Authorization: Bearer) plus-sS --retry 5 --retry-delay 2, replacing the previouseval-built command string that interpolated the token into a shell command.resolve_asset_urllooks up the asset for the current OS/arch viaGET /releases/tags/<version>and downloads it fromGET /releases/assets/<id>withAccept: application/octet-stream. That lands in the documented authenticated rate limit bucket instead of an undocumented one.releases/downloadURL, so external consumers are unaffected.Docs referenced: rate limits for the REST API, release assets endpoints.
Describe alternatives you've considered
releases/downloadURL — verified that this has no effect on the actual byte transfer, since the header is dropped at the redirect.How to test it
Also exercised the partial-version path (
VERSION=v2), the cached-binary path, and an unmatched version.Additional context
Shell-script-only change used by CI, so no UI screenshots apply. The Windows
zippath is unchanged apart from going throughgithub_curl; the archive extension handling is untouched.Link to Devin session: https://app.devin.ai/sessions/98e5024773d947509ad2575ae80bf31b
Requested by: @kinyoklion