Repository navigation
Conversation
A non-200 response read its whole body into SubscriptionError.Message with io.ReadAll, and Error included that body verbatim. The body comes from whatever answered the request, which may be an intermediary rather than the intended service. Two consequences for a caller that logs the error, which every consumer that passes a logger to this library does. Control characters in the body end the caller's log line and start lines of its own that look just as genuine. An error page of any size is held in memory and reproduced in the log in full, once per retry. The body now reads up to 256 bytes and says so when it was longer. That bound is what a log line can carry: nothing branches on the body, so the only thing that consumes it is a person reading one line. Error replaces every control character with a space. The Message field still holds the body as it was sent, so a caller that wants it has it.
17f6fed to
e09c9dc
Compare
|
Closing. The severity does not justify the change. Forging a log line requires controlling the body of a TLS-protected request, so the realistic sources are the customer's own inspecting proxy, a misconfigured stream URI, or LaunchDarkly itself -- and the payoff is a fake entry in the customer's own log. No disclosure, no escalation. The volume argument is weaker: a rejected key retries every five minutes to an hour under Relay's new backoff, so even a large error page is nothing next to normal log traffic. Two mundane arguments do survive -- an unbounded Unrelated and still open: #76 fixes a genuine |
1 participant
Summary
A non-200 response reads its whole body into
SubscriptionError.Messagewith an unboundedio.ReadAll, andError()interpolates that body into the error string verbatim. The body comes from whatever answered the request, which may be an intermediary rather than the intended service.Every consumer that hands this library a logger writes that string to its log, so two things follow:
TestSubscriptionErrorStringCannotSpanLinesandTestRetryLogLineCannotSpanLinesboth fail without the fix, the second on the line this library logs itself (Connection failed (%s), retrying in %0.4f secs).Messagenow reads up to 256 bytes and is marked when the body was longer.Error()replaces every control character with a space and trims the result. The field still holds the body as it was sent, so a caller that wants the raw text has it -- only its length is constrained.Why 256
Nothing branches on the body. Both go-server-sdk data sources and all three Relay sites take
CodeandHeaderoff the error and ignoreMessage; FDv2 even signals its FDv1 fallback through a header. So the only thing that consumes the body is a person reading a log line, which makes "how long should a log line be" the whole question.256 bytes leaves an order of magnitude of headroom over a real service error (
{"message":"..."}is tens of bytes) and still captures the useful part of the bad cases: an nginx 502 body is about 150 bytes end to end, and a load balancer's error page carries its title in the first hundred. Neither this library nor js-core, java-core, or dotnet-core bounds an error body today, so there was no precedent to match.Who this affects
Both
go-server-sdkdata sources pass the SDK's logger straight through (internal/datasource/streaming_data_source.go:408andinternal/datasourcev2/streaming_data_source.go:454in v7.17.0), as does the Relay Proxy for its auto-configuration stream. So this reaches every Go SDK user today: the error handler in each of those consumers only stops the stream for a401or403, and every other non-200 -- a404, any5xx-- retries and logs.Found while working on Relay Proxy auto-configuration retry behavior. A wrapper in Relay was the first attempt; it was the wrong layer, since it would have fixed one of three consumers in the same process and left the SDKs untouched.
Severity
Low. TLS means an on-path attacker cannot inject a body. The realistic sources are an inspecting proxy the customer deployed, a misconfigured stream URI, or the service itself. The impact is a forged log entry and log volume, not disclosure.
Note on scope
Messageis a public field, and capping it is a behavior change for any caller that logs the field directly rather than the error. The library's own example does exactly that (example_error_handling_stream_test.go), so this needs a release note. Retaining an unbounded remote string is the thing being fixed, but a caller relying on the whole body should hear about it.mainalready reports four golangci-lint findings (1 gocritic, 2 gofmt, 1 gosec) at90772ec. This branch reports the same four and no others; I left them alone.