fix: sanitize URL to prevent leaking user credentials in logs #6175
Conversation
This file contains 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
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## unstable #6175 +/- ##
=============================================
+ Coverage 80.02% 90.35% +10.33%
=============================================
Files 19 78 +59
Lines 1717 8089 +6372
Branches 155 490 +335
=============================================
+ Hits 1374 7309 +5935
- Misses 341 772 +431
- Partials 2 8 +6 |
jeluard
previously approved these changes
Dec 9, 2023
| * Sanitize URL to prevent leaking user credentials in logs | ||
| */ | ||
| export function toSafePrintableUrl(urlStr: string): string { | ||
| return new URL(urlStr).origin; |
There was a problem hiding this comment.
This will strip out searchpart of URL, assuming this is fine.
There was a problem hiding this comment.
This is desired in case someone provides secrets in query params. I don't know of anybody that does this but we can't prevent users from doing that and passing credentials directly as part of the URL is common because it works with all clients.
There was a problem hiding this comment.
Got it, somehow I assumed credentials could be passed via URL username/password properties
There was a problem hiding this comment.
Note that calling URL with an invalid URL with throw an error. Is urlStr sure to be a valid URL in the normal Beacon Node init flow? Else users may get funny errors with the wrong URL.
There was a problem hiding this comment.
Good catch! Completely forgot about that, I added a note to the function now and moved all URL logs after they are already validated by http / jsonrpc client.
Also looked into sanitizing invalid URLs (in catch block) but that got too messy.
Performance Report✔️ no performance regression detected 🚀🚀 Significant benchmark improvement detected
Full benchmark results
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
nflaig
commented
Dec 11, 2023
| @@ -59,6 +58,7 @@ export class ExecutionBuilderHttp implements IExecutionBuilder { | |||
| }, | |||
| {config, metrics: metrics?.builderHttpClient} | |||
| ); | |||
| logger?.info("External builder", {url: toSafePrintableUrl(baseUrl)}); | |||
There was a problem hiding this comment.
Builder is a bit of an outlier right now, we allow to set multiple URLs via --builder.urls but only use the first URL (opts.urls[0]). I opted to only log the URL which is actually used.
@g11tech Is it intentional that we don't allow a fallback for the builder? I guess mev-boost is already doing the multiplexing and builder flow is stateful which makes fallback logic a bit problematic.
Maybe we want to reflect that in the CLI as well as currently it looks like it supports multiple urls
wemeetagain
approved these changes
Dec 12, 2023
|
🎉 This PR is included in v1.13.0 🎉 |
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.
Motivation
Recently added printing out URLs of connected services (#6099). Those might contain user credentials, e.g. if validator is connected to rescue node. We should make sure to sanitize URLs before printing those in logs to prevent potentially leaking those credentials.
Description
Sanitize URL to prevent leaking user credentials in logs