SDK-3246: full support for RFC9207 in node (iss parameter) (#3225)

The main issue is that openid-client will compare the provided callback URL to the URL provided in the Authorization request, and we weren't cleaning up the iss parameter from the effective callback URL, causing a mismatch.

This include some refactoring, moving the openid cleanup util from the browser module to the code module. I've also removed a related test for the openid params specific to the implicit flow, which we don't support, and code to implement the actual check of RFC 9207, which is supported by openid-client.

* Remove OpenID parameters from request URL in the core module
* Update openid-browser, node and browser lib to use shared openid cleanup
* RFC9207 is implemented by oidc-client
* Update node lib to use shared openid cleanup
* Remove cleanup and tests for unsupported implicit flow

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
+
+#### node
+
+- [Support for `iss` parameter](https://github.com/inrupt/solid-client-authn-js/issues/2985): [RFC9207](https://www.rfc-editor.org/rfc/rfc9207) adds
+  an `iss` parameter to the callback IRI, and the Node library wasn't removing it before comparing the effective callback URL to the URL used
+  when making the Authorization request.
+
 ## [1.17.3](https://github.com/inrupt/solid-client-authn-js/releases/tag/v1.17.3) - 2023-10-10
 
 ### Bugfixes

packages/browser/src/ClientAuthentication.spec.ts

@@ -493,25 +493,6 @@ describe("ClientAuthentication", () => {
       );
     });
 
-    it("clears the current IRI from OAuth query parameters in the implicit flow", async () => {
-      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);
-      expect(window.history.replaceState).toHaveBeenCalledWith(
-        null,
-        "",
-        "https://coolapp.com/redirect",
-      );
-      expect(mockEmitter.emit).not.toHaveBeenCalled();
-    });
-
     it("preserves non-OAuth query strings", async () => {
       window.history.replaceState = jest
         .fn<typeof window.history.replaceState>()

packages/browser/src/ClientAuthentication.ts

@@ -33,8 +33,9 @@ import {
   EVENTS,
   isValidRedirectUrl,
   ClientAuthentication as ClientAuthenticationBase,
+  removeOpenIdParams,
 } from "@inrupt/solid-client-authn-core";
-import { removeOidcQueryParam } from "@inrupt/oidc-client-ext";
+import { normalizeCallbackUrl } from "@inrupt/oidc-client-ext";
 import type { EventEmitter } from "events";
 
 /**
@@ -62,7 +63,7 @@ export default class ClientAuthentication extends ClientAuthenticationBase {
     // normalization only applies if we default to the current location (which is
     // a bad practice and should be discouraged).
     const redirectUrl =
-      options.redirectUrl ?? removeOidcQueryParam(window.location.href);
+      options.redirectUrl ?? normalizeCallbackUrl(window.location.href);
     if (!isValidRedirectUrl(redirectUrl)) {
       throw new Error(
         `${redirectUrl} is not a valid redirect URL, it is either a malformed IRI, includes a hash fragment, or reserved query parameters ('code' or 'state').`,
@@ -133,25 +134,15 @@ export default class ClientAuthentication extends ClientAuthenticationBase {
   };
 
   private async cleanUrlAfterRedirect(url: string): Promise<void> {
-    const cleanedUpUrl = new URL(url);
-    cleanedUpUrl.searchParams.delete("state");
-    // For auth code flow
-    cleanedUpUrl.searchParams.delete("code");
-    // For implicit flow
-    cleanedUpUrl.searchParams.delete("id_token");
-    cleanedUpUrl.searchParams.delete("access_token");
-    // For login error
-    cleanedUpUrl.searchParams.delete("error");
-    cleanedUpUrl.searchParams.delete("error_description");
-    cleanedUpUrl.searchParams.delete("iss");
+    const cleanedUpUrl = removeOpenIdParams(url).href;
 
     // 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());
-    while (window.location.href !== cleanedUpUrl.href) {
+    window.history.replaceState(null, "", cleanedUpUrl);
+    while (window.location.href !== cleanedUpUrl) {
       // 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

packages/core/src/index.ts

@@ -46,7 +46,10 @@ export { getWebidFromTokenPayload, fetchJwks } from "./util/token";
 
 export { default as IOidcHandler } from "./login/oidc/IOidcHandler";
 export { default as IOidcOptions } from "./login/oidc/IOidcOptions";
-export { isValidRedirectUrl } from "./login/oidc/validateRedirectIri";
+export {
+  isValidRedirectUrl,
+  removeOpenIdParams,
+} from "./login/oidc/redirectIriUtils";
 
 export {
   default as IIncomingRedirectHandler,

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

@@ -34,3 +34,16 @@ export function isValidRedirectUrl(redirectUrl: string): boolean {
     return false;
   }
 }
+
+export function removeOpenIdParams(redirectUrl: string): URL {
+  const cleanedUpUrl = new URL(redirectUrl);
+  // For auth code flow
+  cleanedUpUrl.searchParams.delete("state");
+  cleanedUpUrl.searchParams.delete("code");
+  // For login error
+  cleanedUpUrl.searchParams.delete("error");
+  cleanedUpUrl.searchParams.delete("error_description");
+  // For RFC9207
+  cleanedUpUrl.searchParams.delete("iss");
+  return cleanedUpUrl;
+}

packages/core/src/login/oidc/validateRedirectIri.spec.ts → packages/core/src/login/oidc/redirectIriutils.spec.ts

@@ -20,7 +20,7 @@
 //
 
 import { it, describe, expect } from "@jest/globals";
-import { isValidRedirectUrl } from "./validateRedirectIri";
+import { isValidRedirectUrl, removeOpenIdParams } from "./redirectIriUtils";
 
 describe("isValidRedirectUrl", () => {
   it("returns false if the provided IRI is malformed", () => {
@@ -43,3 +43,25 @@ describe("isValidRedirectUrl", () => {
     ).toBe(true);
   });
 });
+
+describe("removeOpenIdParams", () => {
+  it("removes the auth code query parameters", () => {
+    expect(
+      removeOpenIdParams("https://example.org/callback?code=1234&state=5678")
+        .href,
+    ).toBe("https://example.org/callback");
+  });
+  it("removes the error query parameters", () => {
+    expect(
+      removeOpenIdParams(
+        "https://example.org/callback?error=1234&error_description=5678",
+      ).href,
+    ).toBe("https://example.org/callback");
+  });
+
+  it("removes the RFC9207 query parameters", () => {
+    expect(
+      removeOpenIdParams("https://example.org/callback?iss=1234").href,
+    ).toBe("https://example.org/callback");
+  });
+});

packages/node/src/login/oidc/incomingRedirectHandler/AuthCodeRedirectHandler.spec.ts

@@ -384,7 +384,7 @@ describe("AuthCodeRedirectHandler", () => {
       });
 
       await authCodeRedirectHandler.handle(
-        "https://my.app/redirect?code=someCode&state=someState",
+        "https://my.app/redirect?code=someCode&state=someState&iss=https://example.org/issuer",
       );
 
       expect(callback).toHaveBeenCalledWith(
@@ -471,7 +471,10 @@ describe("AuthCodeRedirectHandler", () => {
     it("throws if the iss parameter does not match stored issuer", async () => {
       const mockedStorage = mockDefaultRedirectStorage();
       const mockedTokens = mockDpopTokens();
-      setupOidcClientMock(mockedTokens);
+      // oidc-client will throw if the iss parameter mismatches.
+      setupOidcClientMock(mockedTokens, () => {
+        throw new Error();
+      });
       const authCodeRedirectHandler = getAuthCodeRedirectHandler({
         storageUtility: mockedStorage,
         sessionInfoManager: mockSessionInfoManager(mockedStorage),
@@ -481,11 +484,7 @@ describe("AuthCodeRedirectHandler", () => {
         authCodeRedirectHandler.handle(
           "https://my.app/redirect?code=someCode&state=someState&iss=someIssuer",
         ),
-      ).rejects.toThrow(
-        `The value of the iss parameter (someIssuer) does not match the issuer identifier of the authorization server (${
-          mockDefaultIssuerConfig().issuer
-        }). See [rfc9207](https://www.rfc-editor.org/rfc/rfc9207.html#section-2.3-3.1.1)`,
-      );
+      ).rejects.toThrow();
     });
 
     it("throws if the IdP does not return an access token", async () => {

packages/node/src/login/oidc/incomingRedirectHandler/AuthCodeRedirectHandler.ts

@@ -45,6 +45,7 @@ import {
   buildAuthenticatedFetch,
   EVENTS,
   maybeBuildRpInitiatedLogout,
+  removeOpenIdParams,
 } from "@inrupt/solid-client-authn-core";
 // eslint-disable-next-line no-shadow
 import { URL } from "url";
@@ -101,8 +102,6 @@ export class AuthCodeRedirectHandler implements IIncomingRedirectHandler {
     const url = new URL(inputRedirectUrl);
     // The type assertion is ok, because we checked in canHandle for the presence of a state
     const oauthState = url.searchParams.get("state") as string;
-    url.searchParams.delete("code");
-    url.searchParams.delete("state");
 
     const sessionId = await getSessionIdFromOauthState(
       this.storageUtility,
@@ -142,7 +141,7 @@ export class AuthCodeRedirectHandler implements IIncomingRedirectHandler {
       dpopKey = await generateDpopKeyPair();
     }
     const tokenSet = await client.callback(
-      url.href,
+      removeOpenIdParams(inputRedirectUrl).href,
       params,
       { code_verifier: oidcContext.codeVerifier, state: oauthState },
       // The KeyLike type is dynamically bound to either KeyObject or CryptoKey
@@ -151,14 +150,6 @@ export class AuthCodeRedirectHandler implements IIncomingRedirectHandler {
       { DPoP: dpopKey?.privateKey as KeyObject },
     );
 
-    const iss = url.searchParams.get("iss");
-
-    if (typeof iss === "string" && iss !== oidcContext.issuerConfig.issuer) {
-      throw new Error(
-        `The value of the iss parameter (${iss}) does not match the issuer identifier of the authorization server (${oidcContext.issuerConfig.issuer}). See [rfc9207](https://www.rfc-editor.org/rfc/rfc9207.html#section-2.3-3.1.1)`,
-      );
-    }
-
     if (
       tokenSet.access_token === undefined ||
       tokenSet.id_token === undefined

packages/oidc-browser/src/cleanup/cleanup.spec.ts

@@ -21,7 +21,7 @@
 
 import { jest, it, describe, expect } from "@jest/globals";
 import OidcClient from "@inrupt/oidc-client";
-import { removeOidcQueryParam, clearOidcPersistentStorage } from "./cleanup";
+import { normalizeCallbackUrl, clearOidcPersistentStorage } from "./cleanup";
 
 jest.mock("@inrupt/oidc-client", () => {
   const mockClient = {
@@ -37,50 +37,50 @@ jest.mock("@inrupt/oidc-client", () => {
 
 describe("removeOidcQueryParam", () => {
   it("removes the 'code' query string if present", () => {
-    expect(removeOidcQueryParam("https://some.url/?code=aCode")).toBe(
+    expect(normalizeCallbackUrl("https://some.url/?code=aCode")).toBe(
       "https://some.url/",
     );
   });
 
   it("removes the 'state' query string if present", () => {
-    expect(removeOidcQueryParam("https://some.url?state=arkansas")).toBe(
+    expect(normalizeCallbackUrl("https://some.url?state=arkansas")).toBe(
       "https://some.url",
     );
   });
 
   it("removes the hash part of the IRI", () => {
-    expect(removeOidcQueryParam("https://some.url/#some-anchor")).toBe(
+    expect(normalizeCallbackUrl("https://some.url/#some-anchor")).toBe(
       "https://some.url/",
     );
   });
 
   it("returns an URL without query strings as is", () => {
-    expect(removeOidcQueryParam("https://some.url/")).toBe("https://some.url/");
+    expect(normalizeCallbackUrl("https://some.url/")).toBe("https://some.url/");
   });
 
   it("does not normalize the trailing slash", () => {
-    expect(removeOidcQueryParam("https://some.url?state=ohio")).toBe(
+    expect(normalizeCallbackUrl("https://some.url?state=ohio")).toBe(
       "https://some.url",
     );
   });
 
   it("preserves the path", () => {
     expect(
-      removeOidcQueryParam("https://coolapp.test/some/redirect?state=ohio"),
+      normalizeCallbackUrl("https://coolapp.test/some/redirect?state=ohio"),
     ).toBe("https://coolapp.test/some/redirect");
   });
 
   it("preserves other query strings", () => {
     expect(
-      removeOidcQueryParam(
+      normalizeCallbackUrl(
         "https://some.url/?code=someCode&state=someState&otherQuery=aValue",
       ),
     ).toBe("https://some.url/?otherQuery=aValue");
   });
 
   it("preserves other query strings when no trailing slash is present", () => {
     expect(
-      removeOidcQueryParam(
+      normalizeCallbackUrl(
         "https://some.url?code=someCode&state=someState&otherQuery=aValue",
       ),
     ).toBe("https://some.url?otherQuery=aValue");

packages/oidc-browser/src/cleanup/cleanup.ts

@@ -20,17 +20,16 @@
 //
 
 import { OidcClient, WebStorageStateStore } from "@inrupt/oidc-client";
+import { removeOpenIdParams } from "@inrupt/solid-client-authn-core";
 
 /**
  * Removes OIDC-specific query parameters from a given URL (state, code...), and
  * sanitizes the URL (e.g. removes the hash fragment).
  * @param redirectUrl The URL to clean up.
  * @returns A copy of the URL, without OIDC-specific query params.
  */
-export function removeOidcQueryParam(redirectUrl: string): string {
-  const cleanedUrl = new URL(redirectUrl);
-  cleanedUrl.searchParams.delete("code");
-  cleanedUrl.searchParams.delete("state");
+export function normalizeCallbackUrl(redirectUrl: string): string {
+  const cleanedUrl = removeOpenIdParams(redirectUrl);
   // As per https://tools.ietf.org/html/rfc6749#section-3.1.2, the redirect URL
   // must not include a hash fragment.
   cleanedUrl.hash = "";

packages/oidc-browser/src/index.ts

@@ -50,6 +50,6 @@ export {
 } from "./dpop/tokenExchange";
 export { refresh } from "./refresh/refreshGrant";
 export {
-  removeOidcQueryParam,
+  normalizeCallbackUrl,
   clearOidcPersistentStorage,
 } from "./cleanup/cleanup";