Fix "Mismatching redirect URL" (#3103)

* Forbid invalid redirect URLs: The client could register redirect URLs including OpenID Connect query
parameters.
* Clear storage on invalid redirect URL persisted: If the stored redirect URL is invalid, the silent login will end up in
an error. To prevent this crash from happening, when an invalid redirect
URL, the storage is cleared, so that the user has to properly log back
in.

CHANGELOG.md

@@ -23,9 +23,17 @@ 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: this bug was caused by an invalid redirect URL stored with session data.
+  Saving an invalid redirect URL is now prohibited, and in addition the storage of users impacted by this bug will be cleared so that they don't have to do anything manually
+  to clear their local storage. Users affected by this bug will be asked to log back in, as if they logged out.
+
 ## [1.17.1](https://github.com/inrupt/solid-client-authn-js/releases/tag/v1.17.1) - 2023-07-15
 
-#### Bugfixes
+### Bugfixes
 
 - The `fetch` function is now bound to the window object in all uses within `authn-browser`
 

packages/browser/src/ClientAuthentication.spec.ts

@@ -274,6 +274,34 @@ describe("ClientAuthentication", () => {
       ).rejects.toThrow("hash fragment");
     });
 
+    it("throws if the redirect IRI contains a reserved query parameter, with a helpful message", async () => {
+      const clientAuthn = getClientAuthentication();
+      await expect(() =>
+        clientAuthn.login(
+          {
+            sessionId: "someUser",
+            tokenType: "DPoP",
+            clientId: "coolApp",
+            redirectUrl: "https://example.org/redirect?state=1234",
+            oidcIssuer: "https://idp.com",
+          },
+          mockEmitter,
+        ),
+      ).rejects.toThrow("query parameter");
+      await expect(() =>
+        clientAuthn.login(
+          {
+            sessionId: "someUser",
+            tokenType: "DPoP",
+            clientId: "coolApp",
+            redirectUrl: "https://example.org/redirect?code=1234",
+            oidcIssuer: "https://idp.com",
+          },
+          mockEmitter,
+        ),
+      ).rejects.toThrow("query parameter");
+    });
+
     it("does not normalize the redirect URL if provided by the user", async () => {
       const clientAuthn = getClientAuthentication();
       await clientAuthn.login(

packages/browser/src/ClientAuthentication.ts

@@ -65,7 +65,7 @@ export default class ClientAuthentication extends ClientAuthenticationBase {
       options.redirectUrl ?? removeOidcQueryParam(window.location.href);
     if (!isValidRedirectUrl(redirectUrl)) {
       throw new Error(
-        `${redirectUrl} is not a valid redirect URL, it is either a malformed IRI or it includes a hash fragment.`,
+        `${redirectUrl} is not a valid redirect URL, it is either a malformed IRI, includes a hash fragment, or reserved query parameters ('code' or 'state').`,
       );
     }
     await this.loginHandler.handle({

packages/browser/src/sessionInfo/SessionInfoManager.spec.ts

@@ -161,6 +161,28 @@ describe("SessionInfoManager", () => {
       });
     });
 
+    it("returns undefined and clears storage if the redirect URL is invalid", async () => {
+      const sessionId = "commanderCool";
+      const storageMock = new StorageUtility(
+        mockStorage({}),
+        mockStorage({
+          [`solidClientAuthenticationUser:${sessionId}`]: {
+            // The state query parameter is reserved in OpenID.
+            redirectUrl: "https://client.example.org/callback?state=1234",
+          },
+        }),
+      );
+      const spiedClear = jest.spyOn(storageMock, "deleteAllUserData");
+      const sessionManager = getSessionInfoManager({
+        storageUtility: storageMock,
+      });
+
+      const session = await sessionManager.get(sessionId);
+      expect(session).toBeUndefined();
+      expect(spiedClear).toHaveBeenCalledWith(sessionId, { secure: false });
+      expect(spiedClear).toHaveBeenCalledWith(sessionId, { secure: true });
+    });
+
     it("returns undefined if the specified storage does not contain the user", async () => {
       const sessionManager = getSessionInfoManager({
         storageUtility: mockStorageUtility({}, true),

packages/browser/src/sessionInfo/SessionInfoManager.ts

@@ -34,6 +34,7 @@ import {
   isSupportedTokenType,
   clear as clearBase,
   SessionInfoManagerBase,
+  isValidRedirectUrl,
 } from "@inrupt/solid-client-authn-core";
 import { clearOidcPersistentStorage } from "@inrupt/oidc-client-ext";
 
@@ -62,60 +63,54 @@ export class SessionInfoManager
   async get(
     sessionId: string,
   ): Promise<(ISessionInfo & ISessionInternalInfo) | undefined> {
-    const isLoggedIn = await this.storageUtility.getForUser(
-      sessionId,
-      "isLoggedIn",
-      {
+    const [
+      isLoggedIn,
+      webId,
+      clientId,
+      clientSecret,
+      redirectUrl,
+      refreshToken,
+      issuer,
+      tokenType,
+    ] = await Promise.all([
+      this.storageUtility.getForUser(sessionId, "isLoggedIn", {
         secure: true,
-      },
-    );
-
-    const webId = await this.storageUtility.getForUser(sessionId, "webId", {
-      secure: true,
-    });
-
-    const clientId = await this.storageUtility.getForUser(
-      sessionId,
-      "clientId",
-      {
+      }),
+      this.storageUtility.getForUser(sessionId, "webId", {
+        secure: true,
+      }),
+      this.storageUtility.getForUser(sessionId, "clientId", {
         secure: false,
-      },
-    );
-
-    const clientSecret = await this.storageUtility.getForUser(
-      sessionId,
-      "clientSecret",
-      {
+      }),
+      this.storageUtility.getForUser(sessionId, "clientSecret", {
         secure: false,
-      },
-    );
-
-    const redirectUrl = await this.storageUtility.getForUser(
-      sessionId,
-      "redirectUrl",
-      {
+      }),
+      this.storageUtility.getForUser(sessionId, "redirectUrl", {
         secure: false,
-      },
-    );
-
-    const refreshToken = await this.storageUtility.getForUser(
-      sessionId,
-      "refreshToken",
-      {
+      }),
+      this.storageUtility.getForUser(sessionId, "refreshToken", {
         secure: true,
-      },
-    );
-
-    const issuer = await this.storageUtility.getForUser(sessionId, "issuer", {
-      secure: false,
-    });
-
-    const tokenType =
-      (await this.storageUtility.getForUser(sessionId, "tokenType", {
+      }),
+      this.storageUtility.getForUser(sessionId, "issuer", {
+        secure: false,
+      }),
+      this.storageUtility.getForUser(sessionId, "tokenType", {
         secure: false,
-      })) ?? "DPoP";
+      }),
+    ]);
+
+    if (typeof redirectUrl === "string" && !isValidRedirectUrl(redirectUrl)) {
+      // This resolves the issue for people experiencing https://github.com/inrupt/solid-client-authn-js/issues/2891.
+      // An invalid redirect URL is present in the storage, and the session should
+      // be cleared to get a fresh start. This will require the user to log back in.
+      await Promise.all([
+        this.storageUtility.deleteAllUserData(sessionId, { secure: false }),
+        this.storageUtility.deleteAllUserData(sessionId, { secure: true }),
+      ]);
+      return undefined;
+    }
 
-    if (!isSupportedTokenType(tokenType)) {
+    if (tokenType !== undefined && !isSupportedTokenType(tokenType)) {
       throw new Error(`Tokens of type [${tokenType}] are not supported.`);
     }
 
@@ -137,7 +132,8 @@ export class SessionInfoManager
       issuer,
       clientAppId: clientId,
       clientAppSecret: clientSecret,
-      tokenType,
+      // Default the token type to DPoP if unspecified.
+      tokenType: tokenType ?? "DPoP",
     };
   }
 

packages/core/src/login/oidc/validateRedirectIri.ts

@@ -23,9 +23,13 @@ export function isValidRedirectUrl(redirectUrl: string): boolean {
   // If the redirect URL is not a valid URL, an error will be thrown.
   try {
     const urlObject = new URL(redirectUrl);
+    const noReservedQuery =
+      !urlObject.searchParams.has("code") &&
+      !urlObject.searchParams.has("state");
     // As per https://tools.ietf.org/html/rfc6749#section-3.1.2, the redirect URL
     // must not include a hash fragment.
-    return urlObject.hash === "";
+    const noHash = urlObject.hash === "";
+    return noReservedQuery && noHash;
   } catch (e) {
     return false;
   }

packages/node/src/ClientAuthentication.spec.ts

@@ -122,6 +122,34 @@ describe("ClientAuthentication", () => {
       ).rejects.toThrow("hash fragment");
     });
 
+    it("throws if the redirect IRI contains a reserved query parameter, with a helpful message", async () => {
+      const clientAuthn = getClientAuthentication();
+      await expect(() =>
+        clientAuthn.login(
+          "mySession",
+          {
+            tokenType: "DPoP",
+            clientId: "coolApp",
+            redirectUrl: "https://example.org/redirect?state=1234",
+            oidcIssuer: "https://idp.com",
+          },
+          mockEmitter,
+        ),
+      ).rejects.toThrow("query parameter");
+      await expect(() =>
+        clientAuthn.login(
+          "mySession",
+          {
+            tokenType: "DPoP",
+            clientId: "coolApp",
+            redirectUrl: "https://example.org/redirect?code=1234",
+            oidcIssuer: "https://idp.com",
+          },
+          mockEmitter,
+        ),
+      ).rejects.toThrow("query parameter");
+    });
+
     it("does not normalize the redirect URL if provided by the user", async () => {
       const clientAuthn = getClientAuthentication();
       await clientAuthn.login(

packages/node/src/ClientAuthentication.ts

@@ -52,7 +52,7 @@ export default class ClientAuthentication extends ClientAuthenticationBase {
       !isValidRedirectUrl(options.redirectUrl)
     ) {
       throw new Error(
-        `${options.redirectUrl} is not a valid redirect URL, it is either a malformed IRI or it includes a hash fragment.`,
+        `${options.redirectUrl} is not a valid redirect URL, it is either a malformed IRI, includes a hash fragment, or reserved query parameters ('code' or 'state').`,
       );
     }
     const loginReturn = await this.loginHandler.handle({