diff --git a/CHANGELOG.md b/CHANGELOG.md index 84c3e7252a..9d6cfdd934 100644 --- 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 + +#### 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 diff --git a/packages/browser/src/ClientAuthentication.spec.ts b/packages/browser/src/ClientAuthentication.spec.ts index 066b004aa5..782801d96d 100644 --- a/packages/browser/src/ClientAuthentication.spec.ts +++ b/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() - .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() diff --git a/packages/browser/src/ClientAuthentication.ts b/packages/browser/src/ClientAuthentication.ts index ee97ba69e9..acaf5537eb 100644 --- a/packages/browser/src/ClientAuthentication.ts +++ b/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 { - 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 diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index cb7b55a88c..8a42f7afb1 100644 --- a/packages/core/src/index.ts +++ b/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, diff --git a/packages/core/src/login/oidc/validateRedirectIri.ts b/packages/core/src/login/oidc/redirectIriUtils.ts similarity index 79% rename from packages/core/src/login/oidc/validateRedirectIri.ts rename to packages/core/src/login/oidc/redirectIriUtils.ts index 738b1bb2c2..b2450c9882 100644 --- a/packages/core/src/login/oidc/validateRedirectIri.ts +++ b/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; +} diff --git a/packages/core/src/login/oidc/validateRedirectIri.spec.ts b/packages/core/src/login/oidc/redirectIriutils.spec.ts similarity index 71% rename from packages/core/src/login/oidc/validateRedirectIri.spec.ts rename to packages/core/src/login/oidc/redirectIriutils.spec.ts index 8a0819cd1a..f30093b379 100644 --- a/packages/core/src/login/oidc/validateRedirectIri.spec.ts +++ b/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"); + }); +}); diff --git a/packages/node/src/login/oidc/incomingRedirectHandler/AuthCodeRedirectHandler.spec.ts b/packages/node/src/login/oidc/incomingRedirectHandler/AuthCodeRedirectHandler.spec.ts index 52e4b3b2eb..b1d07dab4c 100644 --- a/packages/node/src/login/oidc/incomingRedirectHandler/AuthCodeRedirectHandler.spec.ts +++ b/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 () => { diff --git a/packages/node/src/login/oidc/incomingRedirectHandler/AuthCodeRedirectHandler.ts b/packages/node/src/login/oidc/incomingRedirectHandler/AuthCodeRedirectHandler.ts index a12f7567e2..b395ae88a8 100644 --- a/packages/node/src/login/oidc/incomingRedirectHandler/AuthCodeRedirectHandler.ts +++ b/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 diff --git a/packages/oidc-browser/src/cleanup/cleanup.spec.ts b/packages/oidc-browser/src/cleanup/cleanup.spec.ts index afaa26aa6c..de04e93f53 100644 --- a/packages/oidc-browser/src/cleanup/cleanup.spec.ts +++ b/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,42 +37,42 @@ 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"); @@ -80,7 +80,7 @@ describe("removeOidcQueryParam", () => { 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"); diff --git a/packages/oidc-browser/src/cleanup/cleanup.ts b/packages/oidc-browser/src/cleanup/cleanup.ts index 6ca6dcaa25..4adf1dc12d 100644 --- a/packages/oidc-browser/src/cleanup/cleanup.ts +++ b/packages/oidc-browser/src/cleanup/cleanup.ts @@ -20,6 +20,7 @@ // 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 @@ -27,10 +28,8 @@ import { OidcClient, WebStorageStateStore } from "@inrupt/oidc-client"; * @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 = ""; diff --git a/packages/oidc-browser/src/index.ts b/packages/oidc-browser/src/index.ts index 411a253ae3..be4abb1ad4 100644 --- a/packages/oidc-browser/src/index.ts +++ b/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";