Wait for URL update on redirect (#3163)

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 #2891.
inrupt · Oct 6, 2023 · 139e82d · 139e82d
1 parent 68bb29f
commit 139e82d
Show file tree

Hide file tree

Showing 4 changed files with 72 additions and 35 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -23,6 +23,14 @@ The following have been deprecated, and will be removed in future major releases
 
 The following changes have been implemented but not released yet:
 
+### Bugfixes
+
+#### browser
+
+- [Mismatching redirect URI](https://github.com/inrupt/solid-client-authn-js/issues/2891) on refresh: the root cause of the bug was a race
+  condition because of the asynchronous nature of updating the browser URL. The appropriate event is now awaited for, which should prevent
+  the issue from manifesting.
+
 ## [1.17.2](https://github.com/inrupt/solid-client-authn-js/releases/tag/v1.17.2) - 2023-09-15
 
 ### Bugfixes

diff --git a/packages/browser/src/ClientAuthentication.spec.ts b/packages/browser/src/ClientAuthentication.spec.ts
@@ -336,8 +336,12 @@ describe("ClientAuthentication", () => {
     // TODO: add tests for events & errors
 
     it("reverts back to un-authenticated fetch on logout", async () => {
-      // eslint-disable-next-line no-restricted-globals
-      history.replaceState = jest.fn();
+      window.history.replaceState = jest
+        .fn<typeof window.history.replaceState>()
+        .mockImplementationOnce((_data, _unused, url) => {
+          // Pretend the current location is updated
+          window.location.href = url as string;
+        });
       const clientAuthn = getClientAuthentication();
 
       const unauthFetch = clientAuthn.fetch;
@@ -402,13 +406,17 @@ describe("ClientAuthentication", () => {
     mockEmitter.emit = jest.fn<typeof mockEmitter.emit>();
 
     it("calls handle redirect", async () => {
-      // eslint-disable-next-line no-restricted-globals
-      history.replaceState = jest.fn();
+      window.history.replaceState = jest
+        .fn<typeof window.history.replaceState>()
+        .mockImplementationOnce((_data, _unused, url) => {
+          // Pretend the current location is updated
+          window.location.href = url as string;
+        });
       const expectedResult = SessionCreatorCreateResponse;
       const clientAuthn = getClientAuthentication();
       const unauthFetch = clientAuthn.fetch;
       const url =
-        "https://coolapp.com/redirect?state=userId&id_token=idToken&access_token=accessToken";
+        "https://example.org/redirect?state=userId&id_token=idToken&access_token=accessToken";
       const redirectInfo = await clientAuthn.handleIncomingRedirect(
         url,
         mockEmitter,
@@ -436,8 +444,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<typeof window.history.replaceState>()
+        .mockImplementationOnce((_data, _unused, url) => {
+          // Pretend the current location is updated
+          window.location.href = url as string;
+        });
       const clientAuthn = getClientAuthentication();
       const url =
         "https://coolapp.com/redirect?state=someState&code=someAuthCode&iss=someIssuer";
@@ -452,8 +464,12 @@ describe("ClientAuthentication", () => {
     });
 
     it("clears the current IRI from OAuth query parameters even if auth flow fails", async () => {
-      // eslint-disable-next-line no-restricted-globals
-      history.replaceState = jest.fn();
+      window.history.replaceState = jest
+        .fn<typeof window.history.replaceState>()
+        .mockImplementationOnce((_data, _unused, url) => {
+          // Pretend the current location is updated
+          window.location.href = url as string;
+        });
 
       mockHandleIncomingRedirect.mockImplementationOnce(() =>
         Promise.reject(new Error("Something went wrong")),
@@ -464,8 +480,7 @@ describe("ClientAuthentication", () => {
       const url =
         "https://coolapp.com/redirect?state=someState&code=someAuthCode";
       await clientAuthn.handleIncomingRedirect(url, mockEmitter);
-      // eslint-disable-next-line no-restricted-globals
-      expect(history.replaceState).toHaveBeenCalledWith(
+      expect(window.history.replaceState).toHaveBeenCalledWith(
         null,
         "",
         "https://coolapp.com/redirect",
@@ -479,14 +494,17 @@ describe("ClientAuthentication", () => {
     });
 
     it("clears the current IRI from OAuth query parameters in the implicit flow", async () => {
-      // eslint-disable-next-line no-restricted-globals
-      history.replaceState = jest.fn();
+      window.history.replaceState = jest
+        .fn<typeof window.history.replaceState>()
+        .mockImplementationOnce((_data, _unused, url) => {
+          // Pretend the current location is updated
+          window.location.href = url as string;
+        });
       const clientAuthn = getClientAuthentication();
       const url =
         "https://coolapp.com/redirect?state=someState&id_token=idToken&access_token=accessToken";
       await clientAuthn.handleIncomingRedirect(url, mockEmitter);
-      // eslint-disable-next-line no-restricted-globals
-      expect(history.replaceState).toHaveBeenCalledWith(
+      expect(window.history.replaceState).toHaveBeenCalledWith(
         null,
         "",
         "https://coolapp.com/redirect",
@@ -495,14 +513,17 @@ describe("ClientAuthentication", () => {
     });
 
     it("preserves non-OAuth query strings", async () => {
-      // eslint-disable-next-line no-restricted-globals
-      history.replaceState = jest.fn();
+      window.history.replaceState = jest
+        .fn<typeof window.history.replaceState>()
+        .mockImplementationOnce((_data, _unused, url) => {
+          // Pretend the current location is updated
+          window.location.href = url as string;
+        });
       const clientAuthn = getClientAuthentication();
       const url =
         "https://coolapp.com/redirect?state=someState&code=someAuthCode&someQuery=someValue";
       await clientAuthn.handleIncomingRedirect(url, mockEmitter);
-      // eslint-disable-next-line no-restricted-globals
-      expect(history.replaceState).toHaveBeenCalledWith(
+      expect(window.history.replaceState).toHaveBeenCalledWith(
         null,
         "",
         "https://coolapp.com/redirect?someQuery=someValue",

diff --git 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<void> {
     const cleanedUpUrl = new URL(url);
     cleanedUpUrl.searchParams.delete("state");
     // For auth code flow
@@ -151,5 +151,16 @@ export default class ClientAuthentication extends ClientAuthenticationBase {
     // authentication library will be called again with what are now invalid
     // query parameters!).
     window.history.replaceState(null, "", cleanedUpUrl.toString());
+    while (window.location.href !== cleanedUpUrl.href) {
+      // Poll the current URL every ms. Active polling is required because
+      // window.history.replaceState is asynchronous, but the associated
+      // 'popstate' event which should be listened to is only sent on active
+      // navigation, which we will not have here.
+      // See https://developer.mozilla.org/en-US/docs/Web/API/Window/popstate_event#when_popstate_is_sent
+      // eslint-disable-next-line no-await-in-loop
+      await new Promise<void>((resolve) => {
+        setTimeout(() => resolve(), 1);
+      });
+    }
   }
 }
diff --git a/packages/browser/src/Session.spec.ts b/packages/browser/src/Session.spec.ts
@@ -246,10 +246,9 @@ describe("Session", () => {
     it("uses current window location as default redirect URL", async () => {
       mockLocation("https://some.url");
       const clientAuthentication = mockClientAuthentication();
-      const incomingRedirectHandler = jest.spyOn(
-        clientAuthentication,
-        "handleIncomingRedirect",
-      );
+      const incomingRedirectHandler = jest
+        .spyOn(clientAuthentication, "handleIncomingRedirect")
+        .mockResolvedValue(undefined);
 
       const mySession = new Session({ clientAuthentication });
       await mySession.handleIncomingRedirect();
@@ -259,13 +258,12 @@ describe("Session", () => {
       );
     });
 
-    it("wraps up ClientAuthentication handleIncomingRedirect", async () => {
+    it("wraps ClientAuthentication handleIncomingRedirect", async () => {
       mockLocation("https://some.url");
       const clientAuthentication = mockClientAuthentication();
-      const incomingRedirectHandler = jest.spyOn(
-        clientAuthentication,
-        "handleIncomingRedirect",
-      );
+      const incomingRedirectHandler = jest
+        .spyOn(clientAuthentication, "handleIncomingRedirect")
+        .mockResolvedValue(undefined);
       const mySession = new Session({ clientAuthentication });
       await mySession.handleIncomingRedirect("https://some.url");
       expect(incomingRedirectHandler).toHaveBeenCalled();
@@ -401,10 +399,9 @@ describe("Session", () => {
 
     it("preserves a binding to its Session instance", async () => {
       const clientAuthentication = mockClientAuthentication();
-      const incomingRedirectHandler = jest.spyOn(
-        clientAuthentication,
-        "handleIncomingRedirect",
-      );
+      const incomingRedirectHandler = jest
+        .spyOn(clientAuthentication, "handleIncomingRedirect")
+        .mockResolvedValue(undefined);
       const mySession = new Session({ clientAuthentication });
       const objectWithHandleIncomingRedirect = {
         handleIncomingRedirect: mySession.handleIncomingRedirect,