From 0fde58de5159538c97be47c2143d2168f67bc002 Mon Sep 17 00:00:00 2001 From: Nicolas Ayral Seydoux Date: Mon, 2 Oct 2023 10:51:38 +0200 Subject: [PATCH] Wait for URL update on redirect Updating the URL through the window object is an asynchronous operation, but it has a synchronous signature. This enforces that the code waits on the appropriate event before proceeding, preventing a race condition resulting in the behavior observed in https://github.com/inrupt/solid-client-authn-js/issues/2891. --- .../browser/src/ClientAuthentication.spec.ts | 8 +++++-- packages/browser/src/ClientAuthentication.ts | 23 +++++++++++-------- 2 files changed, 20 insertions(+), 11 deletions(-) diff --git a/packages/browser/src/ClientAuthentication.spec.ts b/packages/browser/src/ClientAuthentication.spec.ts index d70ad97cec..31a0851626 100644 --- a/packages/browser/src/ClientAuthentication.spec.ts +++ b/packages/browser/src/ClientAuthentication.spec.ts @@ -436,8 +436,12 @@ describe("ClientAuthentication", () => { }); it("clears the current IRI from OAuth query parameters in the auth code flow", async () => { - // eslint-disable-next-line no-restricted-globals - history.replaceState = jest.fn(); + window.history.replaceState = jest.fn(); + window.addEventListener = jest + .fn() + .mockImplementation((_event: unknown, callback: unknown) => { + (callback as () => void)(); + }); const clientAuthn = getClientAuthentication(); const url = "https://coolapp.com/redirect?state=someState&code=someAuthCode&iss=someIssuer"; diff --git a/packages/browser/src/ClientAuthentication.ts b/packages/browser/src/ClientAuthentication.ts index 770879be3c..e3e33e47ae 100644 --- a/packages/browser/src/ClientAuthentication.ts +++ b/packages/browser/src/ClientAuthentication.ts @@ -109,7 +109,7 @@ export default class ClientAuthentication extends ClientAuthenticationBase { this.boundLogout = redirectInfo.getLogoutUrl; // Strip the oauth params: - this.cleanUrlAfterRedirect(url); + await this.cleanUrlAfterRedirect(url); return { isLoggedIn: redirectInfo.isLoggedIn, @@ -119,7 +119,7 @@ export default class ClientAuthentication extends ClientAuthenticationBase { }; } catch (err) { // Strip the oauth params: - this.cleanUrlAfterRedirect(url); + await this.cleanUrlAfterRedirect(url); // FIXME: EVENTS.ERROR should be errorCode, errorDescription // @@ -132,7 +132,7 @@ export default class ClientAuthentication extends ClientAuthenticationBase { } }; - private cleanUrlAfterRedirect(url: string): void { + private async cleanUrlAfterRedirect(url: string): Promise { const cleanedUpUrl = new URL(url); cleanedUpUrl.searchParams.delete("state"); // For auth code flow @@ -145,11 +145,16 @@ export default class ClientAuthentication extends ClientAuthenticationBase { cleanedUpUrl.searchParams.delete("error_description"); cleanedUpUrl.searchParams.delete("iss"); - // Remove OAuth-specific query params (since the login flow finishes with - // the browser being redirected back with OAuth2 query params (e.g. for - // 'code' and 'state'), and so if the user simply refreshes this page our - // authentication library will be called again with what are now invalid - // query parameters!). - window.history.replaceState(null, "", cleanedUpUrl.toString()); + return new Promise((resolve) => { + window.addEventListener("popstate", () => { + resolve(); + }); + // Remove OAuth-specific query params (since the login flow finishes with + // the browser being redirected back with OAuth2 query params (e.g. for + // 'code' and 'state'), and so if the user simply refreshes this page our + // authentication library will be called again with what are now invalid + // query parameters!). + window.history.replaceState(null, "", cleanedUpUrl.toString()); + }); } }