fix: ensure oauth parameters are always stripped from URLs

Previously in error conditions, the oauth params were not stripped which lead to continuous errors for the user

CHANGELOG.md

@@ -25,6 +25,10 @@ storage.
 - The refresh flow was broken for browser-based applications using a client identifier, 
 leading to short session lifetime. Now that this is fixed, the background refresh
 will happen normally, and the session will remain active.
+- The incoming redirect sometimes left OAuth parameters on the URL, despite
+already having consumed them, this only happened in certain error scenarios,
+but now the parameters will always be removed, such that the user doesn't get
+stuck at an error.
 
 ## 1.11.7 - 2022-03-16
 

packages/browser/src/ClientAuthentication.spec.ts

@@ -19,13 +19,14 @@
  * SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
  */
 
-import { jest, it, describe, expect } from "@jest/globals";
+import { jest, it, describe, expect, beforeEach } from "@jest/globals";
 import { EventEmitter } from "events";
 import * as SolidClientAuthnCore from "@inrupt/solid-client-authn-core";
 
 import {
   StorageUtility,
   USER_SESSION_PREFIX,
+  EVENTS,
 } from "@inrupt/solid-client-authn-core";
 
 // FIXME: For some reason jest crashes on trying to handle a subpath import
@@ -34,6 +35,7 @@ import {
   mockStorageUtility,
   mockStorage,
   mockIncomingRedirectHandler,
+  mockHandleIncomingRedirect,
 } from "../../core/src/mocks";
 
 import { mockLoginHandler } from "./login/__mocks__/LoginHandler";
@@ -105,8 +107,44 @@ describe("ClientAuthentication", () => {
     );
   }
 
+  beforeEach(() => {
+    jest.clearAllMocks();
+
+    Object.defineProperty(window, "location", {
+      writable: true,
+      value: { assign: jest.fn() },
+    });
+  });
+
   describe("login", () => {
     const mockEmitter = new EventEmitter();
+    // TODO: add tests for events & errors
+
+    it("calls login, and uses the window.location.href for the redirect if no redirectUrl is set", async () => {
+      // Set a current window location which will be the expected URL to be
+      // redirected back to, since we don't pass redirectUrl:
+      window.location.href = "https://coolapp.test/some/redirect";
+
+      const clientAuthn = getClientAuthentication();
+      await clientAuthn.login(
+        {
+          sessionId: "mySession",
+          tokenType: "DPoP",
+          clientId: "coolApp",
+          oidcIssuer: "https://idp.com",
+        },
+        mockEmitter
+      );
+      expect(defaultMocks.loginHandler.handle).toHaveBeenCalledWith({
+        sessionId: "mySession",
+        clientId: "coolApp",
+        redirectUrl: "https://coolapp.test/some/redirect",
+        oidcIssuer: "https://idp.com",
+        clientName: "coolApp",
+        eventEmitter: mockEmitter,
+        tokenType: "DPoP",
+      });
+    });
 
     it("calls login, and defaults to a DPoP token", async () => {
       const clientAuthn = getClientAuthentication();
@@ -208,6 +246,7 @@ describe("ClientAuthentication", () => {
 
   describe("logout", () => {
     const mockEmitter = new EventEmitter();
+    // TODO: add tests for events & errors
 
     it("reverts back to un-authenticated fetch on logout", async () => {
       window.fetch = jest.fn();
@@ -275,6 +314,7 @@ describe("ClientAuthentication", () => {
 
   describe("handleIncomingRedirect", () => {
     const mockEmitter = new EventEmitter();
+    mockEmitter.emit = jest.fn();
 
     it("calls handle redirect", async () => {
       // eslint-disable-next-line no-restricted-globals
@@ -307,6 +347,7 @@ describe("ClientAuthentication", () => {
 
       // Calling the redirect handler should have updated the fetch.
       expect(clientAuthn.fetch).not.toBe(unauthFetch);
+      expect(mockEmitter.emit).not.toHaveBeenCalled();
     });
 
     it("clears the current IRI from OAuth query parameters in the auth code flow", async () => {
@@ -322,6 +363,34 @@ describe("ClientAuthentication", () => {
         "",
         "https://coolapp.com/redirect"
       );
+      expect(mockEmitter.emit).not.toHaveBeenCalled();
+    });
+
+    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();
+
+      mockHandleIncomingRedirect.mockImplementationOnce(() =>
+        Promise.reject(new Error("Something went wrong"))
+      );
+
+      const clientAuthn = getClientAuthentication();
+
+      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(
+        null,
+        "",
+        "https://coolapp.com/redirect"
+      );
+
+      expect(mockEmitter.emit).toHaveBeenCalledWith(
+        EVENTS.ERROR,
+        "redirect",
+        new Error("Something went wrong")
+      );
     });
 
     it("clears the current IRI from OAuth query parameters in the implicit flow", async () => {
@@ -337,6 +406,7 @@ describe("ClientAuthentication", () => {
         "",
         "https://coolapp.com/redirect"
       );
+      expect(mockEmitter.emit).not.toHaveBeenCalled();
     });
 
     it("preserves non-OAuth query strings", async () => {
@@ -352,6 +422,7 @@ describe("ClientAuthentication", () => {
         "",
         "https://coolapp.com/redirect?someQuery=someValue"
       );
+      expect(mockEmitter.emit).not.toHaveBeenCalled();
     });
   });
 

packages/browser/src/ClientAuthentication.ts

@@ -33,6 +33,7 @@ import {
   IIssuerConfigFetcher,
   ISessionInternalInfo,
   ILoginOptions,
+  EVENTS,
 } from "@inrupt/solid-client-authn-core";
 import { removeOidcQueryParam } from "@inrupt/oidc-client-ext";
 import { EventEmitter } from "events";
@@ -129,13 +130,37 @@ export default class ClientAuthentication {
     url: string,
     eventEmitter: EventEmitter
   ): Promise<ISessionInfo | undefined> => {
-    const redirectInfo = await this.redirectHandler.handle(url, eventEmitter);
-    // The `FallbackRedirectHandler` directly returns the global `fetch` for
-    // his value, so we should ensure it's bound to `window` rather than to
-    // ClientAuthentication, to avoid the following error:
-    // > 'fetch' called on an object that does not implement interface Window.
-    this.fetch = redirectInfo.fetch.bind(window);
+    try {
+      const redirectInfo = await this.redirectHandler.handle(url, eventEmitter);
+      // The `FallbackRedirectHandler` directly returns the global `fetch` for
+      // his value, so we should ensure it's bound to `window` rather than to
+      // ClientAuthentication, to avoid the following error:
+      // > 'fetch' called on an object that does not implement interface Window.
+      this.fetch = redirectInfo.fetch.bind(window);
+
+      // Strip the oauth params:
+      this.cleanUrlAfterRedirect(url);
+
+      return {
+        isLoggedIn: redirectInfo.isLoggedIn,
+        webId: redirectInfo.webId,
+        sessionId: redirectInfo.sessionId,
+        expirationDate: redirectInfo.expirationDate,
+      };
+    } catch (err) {
+      // Strip the oauth params:
+      this.cleanUrlAfterRedirect(url);
+
+      // FIXME: EVENTS.ERROR should be errorCode, errorDescription
+      //
+      // I'm not sure if "redirect" is a good error code, and in theory `err`
+      // maybe an Error object and not a string; Maybe we want to just hardcode
+      // a description instead?
+      eventEmitter.emit(EVENTS.ERROR, "redirect", err);
+    }
+  };
 
+  private cleanUrlAfterRedirect(url: string): void {
     const cleanedUpUrl = new URL(url);
     cleanedUpUrl.searchParams.delete("state");
     // For auth code flow
@@ -153,12 +178,5 @@ export default class ClientAuthentication {
     // authentication library will be called again with what are now invalid
     // query parameters!).
     window.history.replaceState(null, "", cleanedUpUrl.toString());
-
-    return {
-      isLoggedIn: redirectInfo.isLoggedIn,
-      webId: redirectInfo.webId,
-      sessionId: redirectInfo.sessionId,
-      expirationDate: redirectInfo.expirationDate,
-    };
-  };
+  }
 }