chore(concurrent-keys): cherry-pick outstanding v8 commits - #787
Open
aaron-zeisler wants to merge 4 commits into
Open
chore(concurrent-keys): cherry-pick outstanding v8 commits#787aaron-zeisler wants to merge 4 commits into
aaron-zeisler wants to merge 4 commits into
Conversation
## Summary Adds `X-LaunchDarkly-Instance-Id` to `browser.DefaultAllowedHeaders` — the `Access-Control-Allow-Headers` value for the browser JS subrouter. ## Context Ensure CORS allowed headers include instance ID Only the browser JS subrouter needs this — the server-side and mobile subrouters bypass CORS. Not blocking today, but keeps Relay consistent with what underlying backend systems now advertise and avoids dropped browser preflights if a browser JS SDK begins sending the header. ## Changes - `internal/browser/cors.go`: append `"X-LaunchDarkly-Instance-Id"` to `DefaultAllowedHeaders`. ## Testing `go test ./internal/browser/... ./internal/middleware/...` → `ok` The existing CORS tests reference `DefaultAllowedHeaders` symbolically, so they cover the new value automatically. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Single-header CORS allowlist change for browser JS with no auth or data-path impact. > > **Overview** > Adds **`X-LaunchDarkly-Instance-Id`** to `browser.DefaultAllowedHeaders`, so browser JS traffic gets that name in **`Access-Control-Allow-Headers`** via `SetCORSHeaders` and the CORS middleware. > > This aligns Relay with backends that advertise the instance ID header and avoids future browser preflight failures if the JS SDK starts sending it. Scope is the browser subrouter only; existing tests that assert against `DefaultAllowedHeaders` pick up the new value automatically. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit d5324d0. 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: Claude Opus 4.8 (1M context) <noreply@anthropic.com> (cherry picked from commit 710b5be)
**Requirements** - [x] I have added test coverage for new or changed functionality - [ ] I have followed the repository's pull request submission guidelines - [ ] I have validated my changes against all supported platform versions **Related issues** [SEC-8503](https://launchdarkly.atlassian.net/browse/SEC-8503) — CSA-01 LaunchDarkly Relay Proxy - Denial of Service (pen test finding). **Describe the solution you've provided** `getClientSideContextProperties` handled `REPORT` requests by reading the entire request body into memory with an unbounded `io.ReadAll(req.Body)`. Because the `http.Server` also has no `ReadTimeout` (intentional, to support streaming endpoints), an attacker could send an arbitrarily large body to the eval endpoints and drive the process to OOM. This adds a new **opt-in** config option that bounds the read: - `maxClientRequestBodySize` / `MAX_CLIENT_REQUEST_BODY_SIZE` on `[Main]` (`ct.OptBase2Bytes`). - **Default is unset = no limit**, preserving existing v8 behavior (backwards compatible). When set, the body is wrapped with `http.MaxBytesReader`, and oversized bodies get `413 Request Entity Too Large` instead of exhausting memory. ```go bodyReader := req.Body if maxBodySize.IsDefined() { bodyReader = http.MaxBytesReader(w, req.Body, int64(maxBodySize.GetOrElse(0))) } body, readErr := io.ReadAll(bodyReader) // errors.As(readErr, &maxBytesErr) -> 413 ``` The value is threaded through `evaluateAllFeatureFlags` → `getClientSideContextProperties` and `pingStreamHandlerWithContext`, so it applies at the single point where the context body is read — covering the client-side (`/sdk/evalx/{envId}/context|user`), mobile (`/msdk/evalx/context|user`), server-side evalx REPORT, and the streaming eval REPORT endpoints. A non-positive value (e.g. `0B`) is now rejected at config load by `validateMaxClientRequestBodySize` (mirroring `validateMaxInboundPayloadSize`), so an invalid limit fails fast at startup rather than silently `413`-ing all eval traffic at runtime. **Note on v9:** per review feedback, on v8 this defaults to no limit for backwards compatibility. In v9 (a major version) we can set a reasonable default (e.g. 5 MiB) that customers can override. **Describe alternatives you've considered** - A hard-coded default limit (first revision): rejected in review because customers may rely on larger bodies and there was no way to change it. - Reusing `Events.MaxInboundPayloadSize`: it defaults to unlimited and is semantically scoped to event payloads. - Setting a server-wide `ReadTimeout`: rejected because the streaming endpoints deliberately keep connections open. **Additional context** `go build`, `go vet`, `golangci-lint run`, and `go test ./relay/... ./config/...` all pass. Tests: `TestReportFlagEvalRejectsOversizedBodyWhenLimitConfigured` (413 when configured), `TestReportFlagEvalAllowsLargeBodyWhenNoLimitConfigured` (unbounded read when unset), and an invalid-config case rejecting `0B` at config load. [SEC-8503]: https://launchdarkly.atlassian.net/browse/SEC-8503 Link to Devin session: https://app.devin.ai/sessions/b730421ffad64b2e9bbeb41caa956ae8 Requested by: @kparkinson-ld (cherry picked from commit 7589634)
(cherry picked from commit 71d475a)
aaron-zeisler
marked this pull request as ready for review
July 31, 2026 22:35
Docker Scout was failing on every v8 image variant (alpine, distroless, distroless-debug) with two findings: - CVE-2026-41178 (medium) in go.opentelemetry.io/otel v1.43.0, fixed in v1.44.0. Bumping otel carried otel/metric and otel/trace to v1.44.0 as well, keeping the family consistent; grpc did not need to move. - GHSA-259r-337f-4rfw in github.com/klauspost/compress v1.18.5, which affects 1.16.0 through 1.18.6 and is fixed in v1.18.7. Dependency-only change: no Relay source changes. (cherry picked from commit efb319f)
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
4 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.
Summary
Brings
feat/concurrent-keysup to date with the functional commits that landed onv8since the branches last shared history atc6dfc7e(the 8.19.5 release), plus the dependency bump that clears the Docker Scout scan. Each is cherry-picked verbatim in date order, preserving the original author and a(cherry picked from commit …)trailer.710b5beX-LaunchDarkly-Instance-Idto the browser CORS allowlist758963471d475aefb319fBackground
An audit of
v8versusfeat/concurrent-keysfound ten commits onv8since the merge base. Six were already present on the feature branch as patch-identical commits, either landed under their own PR number (#770 for eventsource, #772 for the CVE dependency bumps) or applied directly (#708, #711, #727, #740).Of the four remaining, three are functional and are included here. The fourth,
b433cf8(chore(v8): release 8.19.6), is deliberately excluded: it is release bookkeeping that would setrelay/version/version.goback to8.19.6, conflicting with the8.20.0-rc.1this branch carries.The ordering matters. #749 and #750 both touch
config/config_validation.goanddocs/configuration.md, so #750 conflicts if applied on its own. Cherry-picking in date order lets both apply cleanly with no conflict resolution, which means the content here is byte-identical to what shipped onv8.Dependency bump
The first CI run on this branch was green except for
Docker Scout Scan, which flagged CVE-2026-41178 ingo.opentelemetry.io/otel1.43.0 and GHSA-259r-337f-4rfw ingithub.com/klauspost/compress1.18.5. Neither was introduced here — both versions were already pinned onfeat/concurrent-keysand onv8, and this PR originally changed no dependencies.The fix is #789 against
v8, cherry-picked here asefb319fso this branch is not left red while that merges.go.modandgo.sumare byte-identical betweenv8andfeat/concurrent-keys, so it applied cleanly. Note that #789 has not merged yet, so the(cherry picked from commit …)trailer on that commit points at the pre-squash SHA onaaronz/bump-otel-compress-cves-v8.go build, the fullgo test ./...unit suite, and the affected package tests (config,internal/events,internal/browser,internal/relayenv,relay) all pass.go vetreports three pre-existing unkeyed-field warnings ininternal/storeandinternal/streams; those files are untouched here and the same warnings appear onfeat/concurrent-keysbefore this change.