Fix idempotency issue around token refresh (#4730)

* Fix idempotency issue around token refresh

Signed-off-by: Michael Telatynski <[email protected]>

* Iterate

Signed-off-by: Michael Telatynski <[email protected]>

* Iterate

Signed-off-by: Michael Telatynski <[email protected]>

* Iterate

Signed-off-by: Michael Telatynski <[email protected]>

* Improve test

Signed-off-by: Michael Telatynski <[email protected]>

* Iterate

Signed-off-by: Michael Telatynski <[email protected]>

* Iterate

Signed-off-by: Michael Telatynski <[email protected]>

---------

Signed-off-by: Michael Telatynski <[email protected]>

Author: t3chguy

babel.config.cjs

@@ -26,6 +26,7 @@ module.exports = {
         ],
     ],
     plugins: [
+        ["@babel/plugin-proposal-decorators", { version: "2023-11" }],
         "@babel/plugin-transform-numeric-separator",
         "@babel/plugin-transform-class-properties",
         "@babel/plugin-transform-object-rest-spread",

package.json

@@ -72,6 +72,7 @@
         "@babel/core": "^7.12.10",
         "@babel/eslint-parser": "^7.12.10",
         "@babel/eslint-plugin": "^7.12.10",
+        "@babel/plugin-proposal-decorators": "^7.25.9",
         "@babel/plugin-syntax-dynamic-import": "^7.8.3",
         "@babel/plugin-transform-class-properties": "^7.12.1",
         "@babel/plugin-transform-numeric-separator": "^7.12.7",

spec/unit/http-api/fetch.spec.ts

@@ -125,7 +125,7 @@ describe("FetchHttpApi", () => {
         ).resolves.toBe(text);
     });
 
-    it("should send token via query params if useAuthorizationHeader=false", () => {
+    it("should send token via query params if useAuthorizationHeader=false", async () => {
         const fetchFn = jest.fn().mockResolvedValue({ ok: true });
         const api = new FetchHttpApi(new TypedEventEmitter<any, any>(), {
             baseUrl,
@@ -134,19 +134,19 @@ describe("FetchHttpApi", () => {
             accessToken: "token",
             useAuthorizationHeader: false,
         });
-        api.authedRequest(Method.Get, "/path");
+        await api.authedRequest(Method.Get, "/path");
         expect(fetchFn.mock.calls[0][0].searchParams.get("access_token")).toBe("token");
     });
 
-    it("should send token via headers by default", () => {
+    it("should send token via headers by default", async () => {
         const fetchFn = jest.fn().mockResolvedValue({ ok: true });
         const api = new FetchHttpApi(new TypedEventEmitter<any, any>(), {
             baseUrl,
             prefix,
             fetchFn,
             accessToken: "token",
         });
-        api.authedRequest(Method.Get, "/path");
+        await api.authedRequest(Method.Get, "/path");
         expect(fetchFn.mock.calls[0][1].headers["Authorization"]).toBe("Bearer token");
     });
 
@@ -163,7 +163,7 @@ describe("FetchHttpApi", () => {
         expect(fetchFn.mock.calls[0][1].headers["Authorization"]).toBeFalsy();
     });
 
-    it("should ensure no token is leaked out via query params if sending via headers", () => {
+    it("should ensure no token is leaked out via query params if sending via headers", async () => {
         const fetchFn = jest.fn().mockResolvedValue({ ok: true });
         const api = new FetchHttpApi(new TypedEventEmitter<any, any>(), {
             baseUrl,
@@ -172,12 +172,12 @@ describe("FetchHttpApi", () => {
             accessToken: "token",
             useAuthorizationHeader: true,
         });
-        api.authedRequest(Method.Get, "/path", { access_token: "123" });
+        await api.authedRequest(Method.Get, "/path", { access_token: "123" });
         expect(fetchFn.mock.calls[0][0].searchParams.get("access_token")).toBeFalsy();
         expect(fetchFn.mock.calls[0][1].headers["Authorization"]).toBe("Bearer token");
     });
 
-    it("should not override manually specified access token via query params", () => {
+    it("should not override manually specified access token via query params", async () => {
         const fetchFn = jest.fn().mockResolvedValue({ ok: true });
         const api = new FetchHttpApi(new TypedEventEmitter<any, any>(), {
             baseUrl,
@@ -186,11 +186,11 @@ describe("FetchHttpApi", () => {
             accessToken: "token",
             useAuthorizationHeader: false,
         });
-        api.authedRequest(Method.Get, "/path", { access_token: "RealToken" });
+        await api.authedRequest(Method.Get, "/path", { access_token: "RealToken" });
         expect(fetchFn.mock.calls[0][0].searchParams.get("access_token")).toBe("RealToken");
     });
 
-    it("should not override manually specified access token via header", () => {
+    it("should not override manually specified access token via header", async () => {
         const fetchFn = jest.fn().mockResolvedValue({ ok: true });
         const api = new FetchHttpApi(new TypedEventEmitter<any, any>(), {
             baseUrl,
@@ -199,16 +199,16 @@ describe("FetchHttpApi", () => {
             accessToken: "token",
             useAuthorizationHeader: true,
         });
-        api.authedRequest(Method.Get, "/path", undefined, undefined, {
+        await api.authedRequest(Method.Get, "/path", undefined, undefined, {
             headers: { Authorization: "Bearer RealToken" },
         });
         expect(fetchFn.mock.calls[0][1].headers["Authorization"]).toBe("Bearer RealToken");
     });
 
-    it("should not override Accept header", () => {
+    it("should not override Accept header", async () => {
         const fetchFn = jest.fn().mockResolvedValue({ ok: true });
         const api = new FetchHttpApi(new TypedEventEmitter<any, any>(), { baseUrl, prefix, fetchFn });
-        api.authedRequest(Method.Get, "/path", undefined, undefined, {
+        await api.authedRequest(Method.Get, "/path", undefined, undefined, {
             headers: { Accept: "text/html" },
         });
         expect(fetchFn.mock.calls[0][1].headers["Accept"]).toBe("text/html");
@@ -468,4 +468,61 @@ describe("FetchHttpApi", () => {
             ]
         `);
     });
+
+    it("should not make multiple concurrent refresh token requests", async () => {
+        const tokenInactiveError = new MatrixError({ errcode: "M_UNKNOWN_TOKEN", error: "Token is not active" }, 401);
+
+        const deferredTokenRefresh = defer<{ accessToken: string; refreshToken: string }>();
+        const fetchFn = jest.fn().mockResolvedValue({
+            ok: false,
+            status: tokenInactiveError.httpStatus,
+            async text() {
+                return JSON.stringify(tokenInactiveError.data);
+            },
+            async json() {
+                return tokenInactiveError.data;
+            },
+            headers: {
+                get: jest.fn().mockReturnValue("application/json"),
+            },
+        });
+        const tokenRefreshFunction = jest.fn().mockReturnValue(deferredTokenRefresh.promise);
+
+        const api = new FetchHttpApi(new TypedEventEmitter<any, any>(), {
+            baseUrl,
+            prefix,
+            fetchFn,
+            doNotAttemptTokenRefresh: false,
+            tokenRefreshFunction,
+            accessToken: "ACCESS_TOKEN",
+            refreshToken: "REFRESH_TOKEN",
+        });
+
+        const prom1 = api.authedRequest(Method.Get, "/path1");
+        const prom2 = api.authedRequest(Method.Get, "/path2");
+
+        await jest.advanceTimersByTimeAsync(10); // wait for requests to fire
+        expect(fetchFn).toHaveBeenCalledTimes(2);
+        fetchFn.mockResolvedValue({
+            ok: true,
+            status: 200,
+            async text() {
+                return "{}";
+            },
+            async json() {
+                return {};
+            },
+            headers: {
+                get: jest.fn().mockReturnValue("application/json"),
+            },
+        });
+        deferredTokenRefresh.resolve({ accessToken: "NEW_ACCESS_TOKEN", refreshToken: "NEW_REFRESH_TOKEN" });
+
+        await prom1;
+        await prom2;
+        expect(fetchFn).toHaveBeenCalledTimes(4); // 2 original calls + 2 retries
+        expect(tokenRefreshFunction).toHaveBeenCalledTimes(1);
+        expect(api.opts.accessToken).toBe("NEW_ACCESS_TOKEN");
+        expect(api.opts.refreshToken).toBe("NEW_REFRESH_TOKEN");
+    });
 });

spec/unit/http-api/index.spec.ts

@@ -59,11 +59,12 @@ describe("MatrixHttpApi", () => {
         xhr.onreadystatechange?.(new Event("test"));
     });
 
-    it("should fall back to `fetch` where xhr is unavailable", () => {
+    it("should fall back to `fetch` where xhr is unavailable", async () => {
         globalThis.XMLHttpRequest = undefined!;
         const fetchFn = jest.fn().mockResolvedValue({ ok: true, json: jest.fn().mockResolvedValue({}) });
         const api = new MatrixHttpApi(new TypedEventEmitter<any, any>(), { baseUrl, prefix, fetchFn });
         upload = api.uploadContent({} as File);
+        await upload;
         expect(fetchFn).toHaveBeenCalled();
     });
 

spec/unit/matrix-client.spec.ts

@@ -2753,12 +2753,13 @@ describe("MatrixClient", function () {
             // WHEN we call `setAccountData` ...
             const setProm = client.setAccountData(eventType, content);
 
+            await jest.advanceTimersByTimeAsync(10);
             // THEN, the REST call should have happened, and had the correct content
             const lastCall = fetchMock.lastCall("put-account-data");
             expect(lastCall).toBeDefined();
             expect(lastCall?.[1]?.body).toEqual(JSON.stringify(content));
 
-            // Even after waiting a bit, the method should not yet have returned
+            // Even after waiting a bit more, the method should not yet have returned
             await jest.advanceTimersByTimeAsync(10);
             let finished = false;
             setProm.finally(() => (finished = true));

src/http-api/fetch.ts

@@ -31,6 +31,7 @@ import {
 } from "./interface.ts";
 import { anySignal, parseErrorResponse, timeoutSignal } from "./utils.ts";
 import { type QueryDict } from "../utils.ts";
+import { singleAsyncExecution } from "../utils/decorators.ts";
 
 interface TypedResponse<T> extends Response {
     json(): Promise<T>;
@@ -106,6 +107,12 @@ export class FetchHttpApi<O extends IHttpOpts> {
         return this.requestOtherUrl(method, fullUri, body, opts);
     }
 
+    /**
+     * Promise used to block authenticated requests during a token refresh to avoid repeated expected errors.
+     * @private
+     */
+    private tokenRefreshPromise?: Promise<unknown>;
+
     /**
      * Perform an authorised request to the homeserver.
      * @param method - The HTTP method e.g. "GET".
@@ -162,13 +169,17 @@ export class FetchHttpApi<O extends IHttpOpts> {
         }
 
         try {
+            // Await any ongoing token refresh
+            await this.tokenRefreshPromise;
             const response = await this.request<T>(method, path, queryParams, body, opts);
             return response;
         } catch (error) {
             const err = error as MatrixError;
 
             if (err.errcode === "M_UNKNOWN_TOKEN" && !opts.doNotAttemptTokenRefresh) {
-                const shouldRetry = await this.tryRefreshToken();
+                const tokenRefreshPromise = this.tryRefreshToken();
+                this.tokenRefreshPromise = Promise.allSettled([tokenRefreshPromise]);
+                const shouldRetry = await tokenRefreshPromise;
                 // if we got a new token retry the request
                 if (shouldRetry) {
                     return this.authedRequest(method, path, queryParams, body, {
@@ -177,6 +188,7 @@ export class FetchHttpApi<O extends IHttpOpts> {
                     });
                 }
             }
+
             // otherwise continue with error handling
             if (err.errcode == "M_UNKNOWN_TOKEN" && !opts?.inhibitLogoutEmit) {
                 this.eventEmitter.emit(HttpApiEvent.SessionLoggedOut, err);
@@ -193,6 +205,7 @@ export class FetchHttpApi<O extends IHttpOpts> {
      * On success, sets new access and refresh tokens in opts.
      * @returns Promise that resolves to a boolean - true when token was refreshed successfully
      */
+    @singleAsyncExecution
     private async tryRefreshToken(): Promise<boolean> {
         if (!this.opts.refreshToken || !this.opts.tokenRefreshFunction) {
             return false;

src/utils/decorators.ts

@@ -0,0 +1,39 @@
+/*
+Copyright 2025 The Matrix.org Foundation C.I.C.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+/**
+ * Method decorator to ensure that only one instance of the method is running at a time,
+ * and any concurrent calls will return the same promise as the original call.
+ * After execution is complete a new call will be able to run the method again.
+ */
+export function singleAsyncExecution<This, Args extends unknown[], Return>(
+    target: (this: This, ...args: Args) => Promise<Return>,
+): (this: This, ...args: Args) => Promise<Return> {
+    let promise: Promise<Return> | undefined;
+
+    async function replacementMethod(this: This, ...args: Args): Promise<Return> {
+        if (promise) return promise;
+        try {
+            promise = target.call(this, ...args);
+            await promise;
+            return promise;
+        } finally {
+            promise = undefined;
+        }
+    }
+
+    return replacementMethod;
+}

tsconfig-build.json

@@ -5,7 +5,6 @@
         "declarationMap": true,
         "sourceMap": true,
         "noEmit": false,
-        "emitDecoratorMetadata": true,
         "outDir": "./lib",
         "rootDir": "src"
     },

tsconfig.json

@@ -1,7 +1,7 @@
 {
     "compilerOptions": {
         "target": "es2022",
-        "experimentalDecorators": true,
+        "experimentalDecorators": false,
         "esModuleInterop": true,
         "module": "commonjs",
         "moduleResolution": "node",

yarn.lock

@@ -394,6 +394,15 @@
     "@babel/helper-plugin-utils" "^7.25.9"
     "@babel/traverse" "^7.25.9"
 
+"@babel/plugin-proposal-decorators@^7.25.9":
+  version "7.25.9"
+  resolved "https://registry.yarnpkg.com/@babel/plugin-proposal-decorators/-/plugin-proposal-decorators-7.25.9.tgz#8680707f943d1a3da2cd66b948179920f097e254"
+  integrity sha512-smkNLL/O1ezy9Nhy4CNosc4Va+1wo5w4gzSZeLe6y6dM4mmHfYOCPolXQPHQxonZCF+ZyebxN9vqOolkYrSn5g==
+  dependencies:
+    "@babel/helper-create-class-features-plugin" "^7.25.9"
+    "@babel/helper-plugin-utils" "^7.25.9"
+    "@babel/plugin-syntax-decorators" "^7.25.9"
+
 "@babel/plugin-proposal-private-property-in-object@7.21.0-placeholder-for-preset-env.2":
   version "7.21.0-placeholder-for-preset-env.2"
   resolved "https://registry.yarnpkg.com/@babel/plugin-proposal-private-property-in-object/-/plugin-proposal-private-property-in-object-7.21.0-placeholder-for-preset-env.2.tgz#7844f9289546efa9febac2de4cfe358a050bd703"
@@ -420,6 +429,13 @@
   dependencies:
     "@babel/helper-plugin-utils" "^7.12.13"
 
+"@babel/plugin-syntax-decorators@^7.25.9":
+  version "7.25.9"
+  resolved "https://registry.yarnpkg.com/@babel/plugin-syntax-decorators/-/plugin-syntax-decorators-7.25.9.tgz#986b4ca8b7b5df3f67cee889cedeffc2e2bf14b3"
+  integrity sha512-ryzI0McXUPJnRCvMo4lumIKZUzhYUO/ScI+Mz4YVaTLt04DHNSjEUjKVvbzQjZFLuod/cYEc07mJWhzl6v4DPg==
+  dependencies:
+    "@babel/helper-plugin-utils" "^7.25.9"
+
 "@babel/plugin-syntax-dynamic-import@^7.8.3":
   version "7.8.3"
   resolved "https://registry.yarnpkg.com/@babel/plugin-syntax-dynamic-import/-/plugin-syntax-dynamic-import-7.8.3.tgz#62bf98b2da3cd21d626154fc96ee5b3cb68eacb3"