Log query parameters on HTTP requests (#3591)

* Log query parameters on HTTP requests Follow-up to #3485 * Only stringify once See #3591 (comment)
matrix-org · Jul 13, 2023 · 8ef2e84 · 8ef2e84
1 parent d92936f
commit 8ef2e84
Show file tree

Hide file tree

Showing 2 changed files with 14 additions and 8 deletions.
diff --git a/spec/unit/http-api/fetch.spec.ts b/spec/unit/http-api/fetch.spec.ts
@@ -300,7 +300,7 @@ describe("FetchHttpApi", () => {
         const fetchFn = jest.fn().mockReturnValue(deferred.promise);
         jest.spyOn(logger, "debug").mockImplementation(() => {});
         const api = new FetchHttpApi(new TypedEventEmitter<any, any>(), { baseUrl, prefix, fetchFn });
-        const prom = api.requestOtherUrl(Method.Get, "https://server:8448/some/path#fragment?query=param");
+        const prom = api.requestOtherUrl(Method.Get, "https://server:8448/some/path?query=param#fragment");
         jest.advanceTimersByTime(1234);
         deferred.resolve({ ok: true, status: 200, text: () => Promise.resolve("RESPONSE") } as Response);
         await prom;
@@ -310,12 +310,12 @@ describe("FetchHttpApi", () => {
         expect(logger.debug).toHaveBeenCalledTimes(2);
         expect(mocked(logger.debug).mock.calls[0]).toMatchInlineSnapshot(`
             [
-              "FetchHttpApi: --> GET https://server:8448/some/path",
+              "FetchHttpApi: --> GET https://server:8448/some/path?query=xxx",
             ]
         `);
         expect(mocked(logger.debug).mock.calls[1]).toMatchInlineSnapshot(`
             [
-              "FetchHttpApi: <-- GET https://server:8448/some/path [1234ms 200]",
+              "FetchHttpApi: <-- GET https://server:8448/some/path?query=xxx [1234ms 200]",
             ]
         `);
     });

diff --git a/src/http-api/fetch.ts b/src/http-api/fetch.ts
@@ -224,7 +224,7 @@ export class FetchHttpApi<O extends IHttpOpts> {
         body?: Body,
         opts: Pick<IRequestOpts, "headers" | "json" | "localTimeoutMs" | "keepAlive" | "abortSignal"> = {},
     ): Promise<ResponseType<T, O>> {
-        const urlForLogs = this.clearUrlParamsForLogs(url);
+        const urlForLogs = this.sanitizeUrlForLogs(url);
         logger.debug(`FetchHttpApi: --> ${method} ${urlForLogs}`);
 
         const headers = Object.assign({}, opts.headers || {});
@@ -299,17 +299,23 @@ export class FetchHttpApi<O extends IHttpOpts> {
         return res as ResponseType<T, O>;
     }
 
-    private clearUrlParamsForLogs(url: URL | string): string {
+    private sanitizeUrlForLogs(url: URL | string): string {
         try {
             let asUrl: URL;
             if (typeof url === "string") {
                 asUrl = new URL(url);
             } else {
                 asUrl = url;
             }
-            // get just the path to remove any potential url param that could have
-            // some potential secrets
-            return asUrl.origin + asUrl.pathname;
+            // Remove the values of any URL params that could contain potential secrets
+            const sanitizedQs = new URLSearchParams();
+            for (const key of asUrl.searchParams.keys()) {
+                sanitizedQs.append(key, "xxx");
+            }
+            const sanitizedQsString = sanitizedQs.toString();
+            const sanitizedQsUrlPiece = sanitizedQsString ? `?${sanitizedQsString}` : "";
+
+            return asUrl.origin + asUrl.pathname + sanitizedQsUrlPiece;
         } catch (error) {
             // defensive coding for malformed url
             return "??";