From fa702dfeb06eed34cb34c4c305db98f968944a5b Mon Sep 17 00:00:00 2001
From: Jacob Shufro <116244+jshufro@users.noreply.github.com>
Date: Thu, 7 Dec 2023 10:36:27 -0500
Subject: [PATCH] fix: do not uri-encode Basic Auth header contents (#6155)

* fix: do not URI-encode Basic Auth header contents

* fix: do not expose username/password outside httpClient class
---
 packages/api/src/utils/client/httpClient.ts   |  5 +--
 .../api/test/unit/client/httpClient.test.ts   | 35 ++++++++++++++++++-
 .../unit/client/httpClientOptions.test.ts     |  4 +++
 packages/utils/src/validation.ts              | 10 ++++++
 4 files changed, 51 insertions(+), 3 deletions(-)

diff --git a/packages/api/src/utils/client/httpClient.ts b/packages/api/src/utils/client/httpClient.ts
index 2070d8c582f1..65a14ac35742 100644
--- a/packages/api/src/utils/client/httpClient.ts
+++ b/packages/api/src/utils/client/httpClient.ts
@@ -109,7 +109,8 @@ export class HttpClient implements IHttpClient {
   private readonly urlsScore: number[];
 
   get baseUrl(): string {
-    return this.urlsOpts[0].baseUrl;
+    // Don't leak username/password to caller
+    return new URL(this.urlsOpts[0].baseUrl).origin;
   }
 
   /**
@@ -292,7 +293,7 @@ export class HttpClient implements IHttpClient {
       }
       if (url.username || url.password) {
         if (headers["Authorization"] === undefined) {
-          headers["Authorization"] = `Basic ${toBase64(`${url.username}:${url.password}`)}`;
+          headers["Authorization"] = `Basic ${toBase64(decodeURIComponent(`${url.username}:${url.password}`))}`;
         }
         // Remove the username and password from the URL
         url.username = "";
diff --git a/packages/api/test/unit/client/httpClient.test.ts b/packages/api/test/unit/client/httpClient.test.ts
index be6006d9e375..85dd1106b996 100644
--- a/packages/api/test/unit/client/httpClient.test.ts
+++ b/packages/api/test/unit/client/httpClient.test.ts
@@ -1,7 +1,7 @@
 import {IncomingMessage} from "node:http";
 import {expect} from "chai";
 import fastify, {RouteOptions} from "fastify";
-import {ErrorAborted, TimeoutError} from "@lodestar/utils";
+import {ErrorAborted, TimeoutError, toBase64} from "@lodestar/utils";
 import {HttpClient, HttpError} from "../../../src/utils/client/index.js";
 import {HttpStatusCode} from "../../../src/utils/client/httpStatusCode.js";
 
@@ -151,6 +151,39 @@ describe("httpClient json client", () => {
     await httpClient.json(testRoute);
   });
 
+  it("should not URI-encode user credentials in Authorization header", async () => {
+    // Semi exhaustive set of characters that RFC-3986 allows in the userinfo portion of a URI
+    // Notably absent is `%`. See comment on isValidHttpUrl().
+    const username = "A1-._~!$'&\"()*+,;=";
+    const password = "b2-._~!$'&\"()*+,;=";
+    let {baseUrl} = await getServer({
+      ...testRoute,
+      handler: async (req) => {
+        expect(req.headers.authorization).to.equal(`Basic ${toBase64(`${username}:${password}`)}`);
+        return {};
+      },
+    });
+    // Since `new URL()` is what URI-encodes, we have to do string manipulation to set the username/password
+    // First validate the assumption that the URL starts with http://
+    expect(baseUrl.indexOf("http://")).to.equal(0);
+    // We avoid using baseUrl.replace() because it treats $ as a special character
+    baseUrl = `http://${username}:${password}@${baseUrl.substring("http://".length)}`;
+
+    const httpClient = new HttpClient({baseUrl: baseUrl});
+
+    await httpClient.json(testRoute);
+  });
+
+  it("should not leak user credentials in baseUrl getter", () => {
+    const url = new URL("http://localhost");
+    url.username = "user";
+    url.password = "password";
+    const httpClient = new HttpClient({baseUrl: url.toString()});
+
+    expect(httpClient.baseUrl.includes(url.username)).to.be.false;
+    expect(httpClient.baseUrl.includes(url.password)).to.be.false;
+  });
+
   it("should handle aborting request with timeout", async () => {
     const {baseUrl} = await getServer({
       ...testRoute,
diff --git a/packages/api/test/unit/client/httpClientOptions.test.ts b/packages/api/test/unit/client/httpClientOptions.test.ts
index 1bad7c69a406..0409a41f5aa9 100644
--- a/packages/api/test/unit/client/httpClientOptions.test.ts
+++ b/packages/api/test/unit/client/httpClientOptions.test.ts
@@ -83,4 +83,8 @@ describe("HTTPClient options", () => {
   it("Throw if invalid value in urls option", () => {
     expect(() => new HttpClient({urls: ["invalid"]})).to.throw(Error);
   });
+
+  it("Throw if invalid username/password", () => {
+    expect(() => new HttpClient({baseUrl: "http://hasa%:%can'tbedecoded@localhost"})).to.throw(Error);
+  });
 });
diff --git a/packages/utils/src/validation.ts b/packages/utils/src/validation.ts
index 0b47a70e2340..ed8b88c912e8 100644
--- a/packages/utils/src/validation.ts
+++ b/packages/utils/src/validation.ts
@@ -2,6 +2,16 @@ export function isValidHttpUrl(urlStr: string): boolean {
   let url;
   try {
     url = new URL(urlStr);
+
+    // `new URL` encodes the username/password with the userinfo percent-encode set.
+    // This means the `%` character is not encoded, but others are (such as `=`).
+    // If a username/password contain a `%`, they will not be able to be decoded.
+    //
+    // Make sure that we can successfully decode the username and password here.
+    //
+    // Unfortunately this means we don't accept every character supported by RFC-3986.
+    decodeURIComponent(url.username);
+    decodeURIComponent(url.password);
   } catch (_) {
     return false;
   }