fix: do not uri-encode Basic Auth header contents

[jshufro]

Motivation

Basic Auth is supported by all clients, and only Lodestar URI-encodes certain special characters provided in the userinfo portion of the URI. These changes align Lodestar with the other clients.

Additionally, it adds some protection against leaking http uri userinfo segments into the logs, as they could be considered sensitive.

Description

First commit fixes the issue where new URL() would URI-encode the = character by simply calling decodeURIComponent on username/password from the parsed URL.

Re: #6154 (comment)

Certain characters are not allowed in URIs, even in the userinfo field. These seem to align with the new URL() behavior fine:

userinfo      = *( unreserved / pct-encoded / sub-delims / ":" )
unreserved    = ALPHA / DIGIT / "-" / "." / "_" / "~"
pct-encoded   = "%" HEXDIG HEXDIG
sub-delims    = "!" / "$" / "&" / "'" / "(" / ")"
                 / "*" / "+" / "," / ";" / "="

Specifically, @ and # are in gen-delims, and should not be parts of username:password pairs provided in a URI.

Because of the way javascript handles URLs, % will not be encoded, and users providing URIs to lodestar with userinfo that contains a % character should take care to manually encode it as %25.

Closes #6154

Steps to test or reproduce

yarn test

[nflaig]

Thanks for the fix and adding tests for it @jshufro! I left few comments, I mostly think we should keep the change as minimal as possible as we have a refactor ongoing and we want to include this change in a hotfix release.

Specifically, @ and # are in gen-delims, and should not be parts of username:password pairs provided in a URI.

I'd agree with that, should at least be a valid URL. If there is really a requirement to support all character we could let the user supply credentials via separate CLI options (e.g. --rest.auth)

You probably shouldn't log the fully qualified uri- the username/password as somewhat sensitive.

Right, we've added these recently, could just mask credentials in the URL before logging it. In case of misconfiguration, having the unmasked URL as part of the error might be good as credentials might have been the issue. The error is also not logged to the file, just printed to the console.

[nflaig]

If we don't strictly need to extract the credentials before validating the URL I think adding this additional complexity to authorization header managed it not ideal (adding the header in multiple places)

I also noticed the refactor we are doing in #6080 allows to change the URL per request which means this implementation won't work.

We have extracted the set the authorization header there already into a separate function

lodestar/packages/api/src/utils/headers.ts

Line 87 in 13b65fc

export function setAuthorizationHeader(url: URL, headers: Headers, {bearerToken}: {bearerToken?: string}): void {

Will make sure we don't have a regression in the refactor, I definitely should look there how we can maybe avoid base64 encoding header on each request but even then, I haven't benchmarked it but I think it's probably negligible.

[jshufro]

Sounds like it's preferable to simply call decodeURIComponent on username/password before base64 encoding until #6080 is released, in that case. I'll drop the first commit.

[nflaig]

yeah, I feel like that would be the easiest solution right now if it resolves the issues with rescue node. I can rebase the feature branch right after this is merged so we get the change included there as well.

[nflaig]

Looks good to me, thanks for the updates @jshufro!

For reference, there has been some discussion on discord related to this PR.

[wemeetagain]

@jshufro thanks for the contribution

[wemeetagain]

🎉 This PR is included in v1.13.0 🎉