fix: set user credentials in URL as Authorization header #5884
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
nflaig
force-pushed
the
nflaig/set-basic-auth-header
branch
from
d521d05 to
7b17f10
Compare
Performance Report✔️ no performance regression detected Full benchmark results
|
nazarhussain
previously requested changes
Aug 17, 2023
Comment on lines
+293
to
+295
| // Remove the username and password from the URL | ||
| url.username = ""; | ||
| url.password = ""; |
There was a problem hiding this comment.
Would prefer to completely remove these fields.
Suggested change
| // Remove the username and password from the URL | |
| url.username = ""; | |
| url.password = ""; | |
| // Remove the username and password from the URL | |
| delete url.username; | |
| delete url.password; |
There was a problem hiding this comment.
TypeScript won't let me The operand of a 'delete' operator must be optional.
And I think this good because downstream code might assume this is a defined string as type is URL.username: string, likely wouldn't cause issues but I prefer to not mess with the type if I pass it to third party code.
If you create a URL object without credentials you also get empty strings
new URL("https://google.com")
URL {
href: 'https://google.com/',
origin: 'https://google.com',
protocol: 'https:',
username: '',
password: '',
host: 'google.com',
hostname: 'google.com',
port: '',
pathname: '/',
search: '',
searchParams: URLSearchParams {},
hash: ''
}
Comment on lines
+289
to
+292
| if (url.username || url.password) { | ||
| if (headers["Authorization"] === undefined) { | ||
| headers["Authorization"] = `Basic ${toBase64(`${url.username}:${url.password}`)}`; | ||
| } |
There was a problem hiding this comment.
Throw an error if the user sets the Authorization header attribute as well as the username/password as options.
There was a problem hiding this comment.
I was thinking about it but after looking at other http client implementations (e.g. native node http) I figured usually they just have a defined order of precedence, and we don't do that for bearer token either.
From node docs:
Sending an Authorization header will override using the auth option to compute basic authentication.
In our case it would be: explicit header > bearer header > basic auth header
As another side note, if we want to throw an error, this can not be implemented here as it would lazily throw the first time a request is done which is not ideal. We have to throw misconfiguration errors on startup. This is why I also added URL validation in the constructor of the http client, these runtime errors are bad, especially if you use fallback URLs which might not be queried for hours / days and when they do you notice that the URL is invalid.
| @@ -279,6 +286,14 @@ export class HttpClient implements IHttpClient { | |||
| if (bearerToken && headers["Authorization"] === undefined) { | |||
| headers["Authorization"] = `Bearer ${bearerToken}`; | |||
| } | |||
| if (url.username || url.password) { | |||
There was a problem hiding this comment.
This should be a compound condition for both, we can't generate a valid Authorization header otherwise.
There was a problem hiding this comment.
As per RFC (https://datatracker.ietf.org/doc/html/rfc2617) those are optional and can be omitted (*, i.e. 0 or more).
user-pass = userid ":" password
userid = *<TEXT excluding ":">
password = *TEXT
In both cases fetch also throws an error as it does not allow credentials in the URL
fetch("http://user@localhost")
TypeError: Request cannot be constructed from a URL that includes credentials: http://user@localhost
fetch("http://:password@localhost")
> Uncaught:
TypeError: Request cannot be constructed from a URL that includes credentials: http://:password@localhost
| @@ -269,7 +276,7 @@ export class HttpClient implements IHttpClient { | |||
| const timer = this.metrics?.requestTime.startTimer({routeId}); | |||
|
|
|||
| try { | |||
| const url = urlJoin(baseUrl, opts.url) + (opts.query ? "?" + stringifyQuery(opts.query) : ""); | |||
| const url = new URL(urlJoin(baseUrl, opts.url) + (opts.query ? "?" + stringifyQuery(opts.query) : "")); | |||
There was a problem hiding this comment.
This function stringifyQuery uses a third-party library. See if we can replace it with the native node:querystring module.
There was a problem hiding this comment.
We can't as it does not support all formats required to be openapi compliant.
Specifically we want the "repeat" format like this topic=topic1&topic=topic2, although I think comma-separated should be supported by all clients as well
lodestar/packages/api/src/utils/client/format.ts
Lines 5 to 7 in f9c7107
There was a problem hiding this comment.
As another side note, we use the same library for parsing, last time I looked at this, there was no native alternative for it and qs was the best library I could find
lodestar/packages/beacon-node/src/api/rest/base.ts
Lines 51 to 58 in 1ff56f6
| @@ -279,6 +286,14 @@ export class HttpClient implements IHttpClient { | |||
| if (bearerToken && headers["Authorization"] === undefined) { | |||
| headers["Authorization"] = `Bearer ${bearerToken}`; | |||
| } | |||
| if (url.username || url.password) { | |||
| if (headers["Authorization"] === undefined) { | |||
| headers["Authorization"] = `Basic ${toBase64(`${url.username}:${url.password}`)}`; | |||
There was a problem hiding this comment.
If we can move this logic to a utility function generateAuthHeader(username, password), that will be easy to manage in future.
There was a problem hiding this comment.
Was considering that as well but opted against it because
- not reused anywhere else
- simple enough
- format of auth header as defined in RFC will never change
Same argument could be made for setting bearer token above but I think the code is clearer as is.
wemeetagain
approved these changes
Aug 21, 2023
wemeetagain
dismissed
nazarhussain’s stale review
all review comments have been addressed
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
Closes #5881
Description
Note: Basic authorization header is only added in HttpClient and not JsonRpcHttpClient as it implements it's own authentication scheme using JWT as bearer tokens.