Conversation
Relay passes its own logger to the eventsource library. When a connection attempt fails, the library logs the error text, and for a non-200 response that text embeds the entire response body. The body comes from whatever answered the request, which may be an intermediary rather than LaunchDarkly. It can contain newlines, so it can end Relay's log line and start lines of its own that look genuine. An HTML error page is also reproduced in full, once per retry. The logger passed to the library now collapses every control character except tab, so the text cannot span lines, and truncates the line.
Member
Author
|
Closing: wrong layer. The defect is entirely in eventsource -- it does the unbounded Worse, this PR fixed one of three consumers in the same process. Fixed upstream instead: launchdarkly/eventsource#75, same ticket (SDK-3117). Relay picks it up with the next eventsource bump, and every Go SDK user gets it too. |
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
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
Relay passes its own logger to the eventsource library for the auto-configuration stream. When a connection attempt fails, the library logs
Connection failed (%s), retrying in %0.4f secswith the error text, and for a non-200 response that error text embeds the entire response body -- read withio.ReadAllintoSubscriptionError.Message.The body comes from whatever answered the request, which may be an intermediary rather than LaunchDarkly. That has two consequences:
TestResponseBodyCannotForgeALogLineproduces four copies of an attacker-chosenFAKE Error: forged log line, each indistinguishable from a real Relay line.The logger handed to the library now collapses every control character except tab to a space, so the text cannot span lines, and truncates at 200 characters. Nothing else about the line changes.
This is shipped behavior, not new
The error handler returns
CloseNow: trueonly for a401or403. Every other non-200 returnsCloseNow: false, and the library then logs the retry line, so any404or5xxon the auto-configuration stream reaches it today. The log line is the same in eventsource v1.11.1 (stream.go:148, which 8.21.0 ships) and v1.14.0 (stream.go:188).The test uses a
500for that reason: a rejected key closes the stream before the library logs, so a401would not exercise the path on this branch.Origin
Split out of #866 on review. It was bundled there on the mistaken belief that the retry change made the line reachable, which it does not. That change does make it matter more -- a rejected key will log this line every five minutes indefinitely instead of once before shutdown -- but the fix stands on its own and belongs ahead of it.