-
-
Notifications
You must be signed in to change notification settings - Fork 318
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: pass user credentials to events http client #6179
Conversation
@@ -109,8 +109,7 @@ export class HttpClient implements IHttpClient { | |||
private readonly urlsScore: number[]; | |||
|
|||
get baseUrl(): string { | |||
// Don't leak username/password to caller |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- addressed by fix: sanitize URL to prevent leaking user credentials in logs #6175, the
baseUrl
is really only used by the events client which requires the username/password
this.logger.verbose("Subscribing to head event"); | ||
this.api.events | ||
.eventstream([EventType.head], signal, this.onHeadUpdate) | ||
.catch((e) => this.logger.error("Failed to subscribe to head event", {}, e)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated change but noticed that currently errors (400 and 500) result in an uncaught exception.
We are currently not throwing an error if server responds with 401 (or 404), might be something to consider in this PR but I think it's safer to retry in case those errors happen, see #4521 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caller won't receive the exception anymore, so any code after the call to start
will be executed now.
I understand from your comment that it's fine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The caller never received the error as the promise was not handled (see void
in previous implementation).
The eventstream
method does not resolve until connection is aborted which effectively happens if you shut down the validator client
lodestar/packages/api/src/beacon/client/events.ts
Lines 48 to 49 in 959a8af
// And abort resolve the promise so finally {} eventSource.close() | |
signal.addEventListener("abort", () => resolve(), {once: true}); |
But there are cases where it rejects if server responds with 400 or 500 error
// Consider 400 and 500 status errors unrecoverable, close the eventsource |
Previously due to void
, these errors would cause an uncaught exception while now the errors are caught and properly logged
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## unstable #6179 +/- ##
============================================
- Coverage 90.35% 90.35% -0.01%
============================================
Files 78 78
Lines 8089 8087 -2
Branches 490 490
============================================
- Hits 7309 7307 -2
Misses 772 772
Partials 8 8 |
Performance Report✔️ no performance regression detected 🚀🚀 Significant benchmark improvement detected
Full benchmark results
|
🎉 This PR is included in v1.13.0 🎉 |
Motivation
Eventstream API does not work with rescue node at the moment because user credentials are removed from URL which is passed to events client.
lodestar/packages/api/src/beacon/client/index.ts
Line 31 in 959a8af
Description
Pass user credentials to events http client. The
EventSource
node implementation will parse the credentials, remove them from the URL and set them in Authorization header. The browser implementation does not support credentials via URL and headers are generally not supported (whatwg/html#2177). We don't expect anyone to use the rescue node from browser, but it is possible to pass authentication tokens as part of cookies.Closes #4521, this was previously already fixed by calling
new URL(...).origin
on the URL as it removed the extra/
but I am now callingurlJoin
which we also use in http client to join urls/paths.