-
-
Notifications
You must be signed in to change notification settings - Fork 290
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: set user credentials in URL as Authorization header #5884
Changes from all commits
6f02f07
7aa238e
9aa412b
51382d0
4fddfda
7b17f10
1ff56f6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -1,4 +1,4 @@ | ||||||||||||||
import {ErrorAborted, Logger, TimeoutError} from "@lodestar/utils"; | ||||||||||||||
import {ErrorAborted, Logger, TimeoutError, isValidHttpUrl, toBase64} from "@lodestar/utils"; | ||||||||||||||
import {ReqGeneric, RouteDef} from "../index.js"; | ||||||||||||||
import {ApiClientResponse, ApiClientSuccessResponse} from "../../interfaces.js"; | ||||||||||||||
import {fetch, isFetchError} from "./fetch.js"; | ||||||||||||||
|
@@ -123,8 +123,15 @@ export class HttpClient implements IHttpClient { | |||||||||||||
|
||||||||||||||
// opts.baseUrl is equivalent to `urls: [{baseUrl}]` | ||||||||||||||
// unshift opts.baseUrl to urls, without mutating opts.urls | ||||||||||||||
for (const urlOrOpts of [...(baseUrl ? [baseUrl] : []), ...urls]) { | ||||||||||||||
for (const [i, urlOrOpts] of [...(baseUrl ? [baseUrl] : []), ...urls].entries()) { | ||||||||||||||
const urlOpts: URLOpts = typeof urlOrOpts === "string" ? {baseUrl: urlOrOpts, ...allUrlOpts} : urlOrOpts; | ||||||||||||||
|
||||||||||||||
if (!urlOpts.baseUrl) { | ||||||||||||||
throw Error(`HttpClient.urls[${i}] is empty or undefined: ${urlOpts.baseUrl}`); | ||||||||||||||
} | ||||||||||||||
if (!isValidHttpUrl(urlOpts.baseUrl)) { | ||||||||||||||
throw Error(`HttpClient.urls[${i}] must be a valid URL: ${urlOpts.baseUrl}`); | ||||||||||||||
} | ||||||||||||||
// De-duplicate by baseUrl, having two baseUrls with different token or timeouts does not make sense | ||||||||||||||
if (!this.urlsOpts.some((opt) => opt.baseUrl === urlOpts.baseUrl)) { | ||||||||||||||
this.urlsOpts.push(urlOpts); | ||||||||||||||
|
@@ -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) : "")); | ||||||||||||||
|
||||||||||||||
const headers = | ||||||||||||||
extraHeaders && opts.headers ? {...extraHeaders, ...opts.headers} : opts.headers || extraHeaders || {}; | ||||||||||||||
|
@@ -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) { | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be a compound condition for both, we can't generate a valid Authorization header otherwise. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As per RFC (https://datatracker.ietf.org/doc/html/rfc2617) those are optional and can be omitted (
In both cases fetch also throws an error as it does not allow credentials in the URL
|
||||||||||||||
headers["Authorization"] = `Basic ${toBase64(`${url.username}:${url.password}`)}`; | ||||||||||||||
} | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we can move this logic to a utility function There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Was considering that as well but opted against it because
Same argument could be made for setting bearer token above but I think the code is clearer as is. |
||||||||||||||
// Remove the username and password from the URL | ||||||||||||||
Comment on lines
+289
to
+292
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Throw an error if the user sets the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
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. |
||||||||||||||
url.username = ""; | ||||||||||||||
url.password = ""; | ||||||||||||||
} | ||||||||||||||
Comment on lines
+293
to
+295
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would prefer to completely remove these fields.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TypeScript won't let me And I think this good because downstream code might assume this is a defined string as type is 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: ''
} |
||||||||||||||
|
||||||||||||||
this.logger?.debug("HttpClient request", {routeId}); | ||||||||||||||
|
||||||||||||||
|
@@ -291,7 +306,7 @@ export class HttpClient implements IHttpClient { | |||||||||||||
|
||||||||||||||
if (!res.ok) { | ||||||||||||||
const errBody = await res.text(); | ||||||||||||||
throw new HttpError(`${res.statusText}: ${getErrorMessage(errBody)}`, res.status, url); | ||||||||||||||
throw new HttpError(`${res.statusText}: ${getErrorMessage(errBody)}`, res.status, url.toString()); | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
const streamTimer = this.metrics?.streamTime.startTimer({routeId}); | ||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
const hasBufferFrom = typeof Buffer !== "undefined" && typeof Buffer.from === "function"; | ||
|
||
export function toBase64(value: string): string { | ||
return hasBufferFrom ? Buffer.from(value).toString("base64") : btoa(value); | ||
} | ||
|
||
export function fromBase64(value: string): string { | ||
return hasBufferFrom ? Buffer.from(value, "base64").toString("utf8") : atob(value); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
export function isValidHttpUrl(urlStr: string): boolean { | ||
let url; | ||
try { | ||
url = new URL(urlStr); | ||
} catch (_) { | ||
return false; | ||
} | ||
|
||
return url.protocol === "http:" || url.protocol === "https:"; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
import "../setup.js"; | ||
import {expect} from "chai"; | ||
import {toBase64, fromBase64} from "../../src/index.js"; | ||
|
||
describe("toBase64", () => { | ||
it("should encode UTF-8 string as base64 string", () => { | ||
expect(toBase64("user:password")).to.be.equal("dXNlcjpwYXNzd29yZA=="); | ||
}); | ||
}); | ||
|
||
describe("fromBase64", () => { | ||
it("should decode UTF-8 string from base64 string", () => { | ||
expect(fromBase64("dXNlcjpwYXNzd29yZA==")).to.be.equal("user:password"); | ||
}); | ||
}); |
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.
This function
stringifyQuery
uses a third-party library. See if we can replace it with the nativenode:querystring
module.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.
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 welllodestar/packages/api/src/utils/client/format.ts
Lines 5 to 7 in f9c7107
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.
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 findlodestar/packages/beacon-node/src/api/rest/base.ts
Lines 51 to 58 in 1ff56f6