From 8460a62c343edaa25d2e9c70c357b6b84237b92e Mon Sep 17 00:00:00 2001 From: Zwifi Date: Mon, 28 Aug 2023 14:51:44 +0200 Subject: [PATCH] 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 | 10 +- .../browser/src/ClientAuthentication.spec.ts | 28 ++++++ packages/browser/src/ClientAuthentication.ts | 2 +- .../sessionInfo/SessionInfoManager.spec.ts | 22 +++++ .../src/sessionInfo/SessionInfoManager.ts | 92 +++++++++---------- .../src/login/oidc/validateRedirectIri.ts | 6 +- .../node/src/ClientAuthentication.spec.ts | 28 ++++++ packages/node/src/ClientAuthentication.ts | 2 +- 8 files changed, 138 insertions(+), 52 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3e8ee48558..7de5fa1f12 100644 --- a/CHANGELOG.md +++ b/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` diff --git a/packages/browser/src/ClientAuthentication.spec.ts b/packages/browser/src/ClientAuthentication.spec.ts index d6eeed0938..d70ad97cec 100644 --- a/packages/browser/src/ClientAuthentication.spec.ts +++ b/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( diff --git a/packages/browser/src/ClientAuthentication.ts b/packages/browser/src/ClientAuthentication.ts index be59cc977a..770879be3c 100644 --- a/packages/browser/src/ClientAuthentication.ts +++ b/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({ diff --git a/packages/browser/src/sessionInfo/SessionInfoManager.spec.ts b/packages/browser/src/sessionInfo/SessionInfoManager.spec.ts index d679ec7655..125c5a3f21 100644 --- a/packages/browser/src/sessionInfo/SessionInfoManager.spec.ts +++ b/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), diff --git a/packages/browser/src/sessionInfo/SessionInfoManager.ts b/packages/browser/src/sessionInfo/SessionInfoManager.ts index d8cb27bf20..78be29986f 100644 --- a/packages/browser/src/sessionInfo/SessionInfoManager.ts +++ b/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", }; } diff --git a/packages/core/src/login/oidc/validateRedirectIri.ts b/packages/core/src/login/oidc/validateRedirectIri.ts index 14432c945c..738b1bb2c2 100644 --- a/packages/core/src/login/oidc/validateRedirectIri.ts +++ b/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; } diff --git a/packages/node/src/ClientAuthentication.spec.ts b/packages/node/src/ClientAuthentication.spec.ts index c7b886af23..089b4522bb 100644 --- a/packages/node/src/ClientAuthentication.spec.ts +++ b/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( diff --git a/packages/node/src/ClientAuthentication.ts b/packages/node/src/ClientAuthentication.ts index 9707b2cb64..7d9572844e 100644 --- a/packages/node/src/ClientAuthentication.ts +++ b/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({