From de4a1e6d35aaaf50c36650de1fed110610b616e1 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Wed, 15 Apr 2026 10:35:02 +0100 Subject: [PATCH] Switch OIDC to response_mode=fragment (#33100) * Refactor: kill off `parseQs` in favour of URLSearchParams * Consolidate app-load url parameter handling * Switch to responseMode=fragment --- apps/web/src/Lifecycle.ts | 54 ++++---- .../src/components/structures/MatrixChat.tsx | 39 ++---- .../components/structures/auth/SoftLogout.tsx | 19 +-- apps/web/src/utils/oidc/authorize.ts | 25 ++-- apps/web/src/vector/app.tsx | 36 +++-- apps/web/src/vector/index.ts | 8 +- apps/web/src/vector/init.tsx | 6 +- apps/web/src/vector/platform/WebPlatform.ts | 6 +- apps/web/src/vector/routing.ts | 11 +- apps/web/src/vector/url_utils.ts | 129 +++++++++++++++--- apps/web/test/unit-tests/Lifecycle-test.ts | 2 +- .../components/structures/MatrixChat-test.tsx | 84 +++++++----- .../unit-tests/utils/oidc/authorize-test.ts | 10 +- apps/web/test/unit-tests/vector/app-test.ts | 2 +- apps/web/test/unit-tests/vector/init-test.ts | 13 +- .../test/unit-tests/vector/url_utils-test.ts | 71 ++++++---- 16 files changed, 309 insertions(+), 206 deletions(-) diff --git a/apps/web/src/Lifecycle.ts b/apps/web/src/Lifecycle.ts index 78c6cc2de6..d7d139249a 100644 --- a/apps/web/src/Lifecycle.ts +++ b/apps/web/src/Lifecycle.ts @@ -18,7 +18,6 @@ import { decodeBase64, } from "matrix-js-sdk/src/matrix"; import { type AESEncryptedSecretStoragePayload } from "matrix-js-sdk/src/types"; -import { type QueryDict } from "matrix-js-sdk/src/utils"; import { logger } from "matrix-js-sdk/src/logger"; import { type IMatrixClientCreds, MatrixClientPeg, type MatrixClientPegAssignOpts } from "./MatrixClientPeg"; @@ -81,6 +80,7 @@ import { } from "./utils/tokens/tokens"; import { TokenRefresher } from "./utils/oidc/TokenRefresher"; import { checkBrowserSupport } from "./SupportedBrowser"; +import { type URLParams } from "./vector/url_utils.ts"; const HOMESERVER_URL_KEY = "mx_hs_url"; const ID_SERVER_URL_KEY = "mx_is_url"; @@ -148,7 +148,7 @@ interface ILoadSessionOpts { guestIsUrl?: string; ignoreGuest?: boolean; defaultDeviceDisplayName?: string; - fragmentQueryParams?: QueryDict; + urlParams?: URLParams; abortSignal?: AbortSignal; } @@ -187,7 +187,7 @@ export async function loadSession(opts: ILoadSessionOpts = {}): Promise let enableGuest = opts.enableGuest || false; const guestHsUrl = opts.guestHsUrl; const guestIsUrl = opts.guestIsUrl; - const fragmentQueryParams = opts.fragmentQueryParams || {}; + const urlParams = opts.urlParams; const defaultDeviceDisplayName = opts.defaultDeviceDisplayName; if (enableGuest && !guestHsUrl) { @@ -195,12 +195,12 @@ export async function loadSession(opts: ILoadSessionOpts = {}): Promise enableGuest = false; } - if (enableGuest && guestHsUrl && fragmentQueryParams.guest_user_id && fragmentQueryParams.guest_access_token) { + if (enableGuest && guestHsUrl && urlParams?.guest?.guest_user_id && urlParams?.guest?.guest_access_token) { logger.log("Using guest access credentials"); await doSetLoggedIn( { - userId: fragmentQueryParams.guest_user_id as string, - accessToken: fragmentQueryParams.guest_access_token as string, + userId: urlParams.guest.guest_user_id, + accessToken: urlParams.guest.guest_access_token, homeserverUrl: guestHsUrl, identityServerUrl: guestIsUrl, guest: true, @@ -264,38 +264,36 @@ export async function getStoredSessionOwner(): Promise<[string, boolean] | [null * If query string includes OIDC authorization code flow parameters attempt to login using oidc flow * Else, we may be returning from SSO - attempt token login * - * @param {Object} queryParams string->string map of the - * query-parameters extracted from the real query-string of the starting - * URI. + * @param urlParams the parameters read in at app load time from the url * - * @param {string} defaultDeviceDisplayName - * @param {string} fragmentAfterLogin path to go to after a successful login, only used for "Try again" + * @param defaultDeviceDisplayName + * @param fragmentAfterLogin path to go to after a successful login, only used for "Try again" * - * @returns {Promise} promise which resolves to true if we completed the delegated auth login + * @returns promise which resolves to true if we completed the delegated auth login * else false */ export async function attemptDelegatedAuthLogin( - queryParams: QueryDict, + urlParams: URLParams, defaultDeviceDisplayName?: string, fragmentAfterLogin?: string, ): Promise { - if (queryParams.code && queryParams.state) { + if (urlParams.oidc) { console.log("We have OIDC params - attempting OIDC login"); - return attemptOidcNativeLogin(queryParams); + return attemptOidcNativeLogin(urlParams["oidc"]); } - return attemptTokenLogin(queryParams, defaultDeviceDisplayName, fragmentAfterLogin); + return attemptTokenLogin(urlParams["legacy_sso"], defaultDeviceDisplayName, fragmentAfterLogin); } /** * Attempt to login by completing OIDC authorization code flow - * @param queryParams string->string map of the query-parameters extracted from the real query-string of the starting URI. - * @returns Promise that resolves to true when login succceeded, else false + * @param urlParams subset of app-load url parameters relating to oidc auth + * @returns Promise that resolves to true when login succeeded, else false */ -async function attemptOidcNativeLogin(queryParams: QueryDict): Promise { +async function attemptOidcNativeLogin(urlParams: NonNullable): Promise { try { const { accessToken, refreshToken, homeserverUrl, identityServerUrl, idToken, clientId, issuer } = - await completeOidcLogin(queryParams); + await completeOidcLogin(urlParams); const { user_id: userId, @@ -354,22 +352,20 @@ async function getUserIdFromAccessToken( } /** - * @param {QueryDict} queryParams string->string map of the - * query-parameters extracted from the real query-string of the starting - * URI. + @param urlParams subset of app-load url parameters relating to legacy sso auth * - * @param {string} defaultDeviceDisplayName - * @param {string} fragmentAfterLogin path to go to after a successful login, only used for "Try again" + * @param defaultDeviceDisplayName + * @param fragmentAfterLogin path to go to after a successful login, only used for "Try again" * - * @returns {Promise} promise which resolves to true if we completed the token + * @returns promise which resolves to true if we completed the token * login, else false */ export function attemptTokenLogin( - queryParams: QueryDict, + urlParams: URLParams["legacy_sso"], defaultDeviceDisplayName?: string, fragmentAfterLogin?: string, ): Promise { - if (!queryParams.loginToken) { + if (!urlParams?.loginToken) { return Promise.resolve(false); } @@ -384,7 +380,7 @@ export function attemptTokenLogin( } return sendLoginRequest(homeserver, identityServer, "m.login.token", { - token: queryParams.loginToken as string, + token: urlParams.loginToken, initial_device_display_name: defaultDeviceDisplayName, }) .then(async function (creds) { diff --git a/apps/web/src/components/structures/MatrixChat.tsx b/apps/web/src/components/structures/MatrixChat.tsx index ce5bfd4598..e93799fb1a 100644 --- a/apps/web/src/components/structures/MatrixChat.tsx +++ b/apps/web/src/components/structures/MatrixChat.tsx @@ -20,7 +20,6 @@ import { type SyncStateData, type TimelineEvents, } from "matrix-js-sdk/src/matrix"; -import { type QueryDict } from "matrix-js-sdk/src/utils"; import { logger } from "matrix-js-sdk/src/logger"; import { throttle } from "lodash"; import { CryptoEvent, type KeyBackupInfo } from "matrix-js-sdk/src/crypto-api"; @@ -141,6 +140,8 @@ import Markdown from "../../Markdown"; import { LinkedTextConfiguration, sanitizeHtmlParams } from "../../Linkify"; import { isOnlyAdmin } from "../../utils/membership"; import { ModuleApi } from "../../modules/Api.ts"; +import { type IScreen } from "../../vector/routing.ts"; +import { type URLParams } from "../../vector/url_utils.ts"; // legacy export export { default as Views } from "../../Views"; @@ -152,21 +153,14 @@ const AUTH_SCREENS = ["register", "mobile_register", "login", "forgot_password", // re-factoring to be included in this list in future. const ONBOARDING_FLOW_STARTERS = [Action.ViewUserSettings, Action.CreateChat, Action.CreateRoom]; -interface IScreen { - screen: string; - params?: QueryDict; -} - interface IProps { config: ConfigOptions; onNewScreen: (screen: string, replaceLast: boolean) => void; enableGuest?: boolean; - // the queryParams extracted from the [real] query-string of the URI - realQueryParams: QueryDict; - // the initial queryParams extracted from the hash-fragment of the URI - startingFragmentQueryParams?: QueryDict; + // the params extracted from the [real] query-string & fragment of the URI + urlParams: URLParams; // called when we have completed a token login - onTokenLoginCompleted: () => void; + onTokenLoginCompleted: (urlParams: URLParams, fragmentAfterLogin: string) => void; // Represents the screen to display as a result of parsing the initial window.location initialScreenAfterLogin?: IScreen; // displayname, if any, to set on the device when logging in/registering. @@ -227,11 +221,8 @@ interface IState { export default class MatrixChat extends React.PureComponent { public static displayName = "MatrixChat"; - public static defaultProps = { - realQueryParams: {}, - startingFragmentQueryParams: {}, + public static defaultProps: Partial = { config: {}, - onTokenLoginCompleted: (): void => {}, }; private firstSyncComplete = false; @@ -353,18 +344,14 @@ export default class MatrixChat extends React.PureComponent { // Otherwise, the first thing to do is to try the token params in the query-string const delegatedAuthSucceeded = await Lifecycle.attemptDelegatedAuthLogin( - this.props.realQueryParams, + this.props.urlParams, this.props.defaultDeviceDisplayName, this.getFragmentAfterLogin(), ); // remove the loginToken or auth code from the URL regardless - if ( - this.props.realQueryParams?.loginToken || - this.props.realQueryParams?.code || - this.props.realQueryParams?.state - ) { - this.props.onTokenLoginCompleted(); + if (!!this.props.urlParams.legacy_sso || !!this.props.urlParams.oidc) { + this.props.onTokenLoginCompleted(this.props.urlParams, this.getFragmentAfterLogin()); } if (delegatedAuthSucceeded) { @@ -592,7 +579,7 @@ export default class MatrixChat extends React.PureComponent { return Promise.resolve() .then(() => { return Lifecycle.loadSession({ - fragmentQueryParams: this.props.startingFragmentQueryParams, + urlParams: this.props.urlParams, enableGuest: this.props.enableGuest, guestHsUrl: this.getServerProperties().serverConfig.hsUrl, guestIsUrl: this.getServerProperties().serverConfig.isUrl, @@ -1835,7 +1822,7 @@ export default class MatrixChat extends React.PureComponent { } } - public showScreen(screen: string, params?: { [key: string]: any }): void { + public showScreen(screen: string, params?: Record): void { logger.debug(`showScreen ${screen}`); const cli = MatrixClientPeg.get(); @@ -2267,14 +2254,14 @@ export default class MatrixChat extends React.PureComponent { onForgotPasswordClick={showPasswordReset ? this.onForgotPasswordClick : undefined} onServerConfigChange={this.onServerConfigChange} fragmentAfterLogin={fragmentAfterLogin} - defaultUsername={this.props.startingFragmentQueryParams?.defaultUsername as string | undefined} + defaultUsername={this.props.urlParams?.defaults?.defaultUsername} {...this.getServerProperties()} /> ); } else if (this.state.view === Views.SOFT_LOGOUT) { view = ( diff --git a/apps/web/src/components/structures/auth/SoftLogout.tsx b/apps/web/src/components/structures/auth/SoftLogout.tsx index a307078658..95d83e9d2b 100644 --- a/apps/web/src/components/structures/auth/SoftLogout.tsx +++ b/apps/web/src/components/structures/auth/SoftLogout.tsx @@ -26,6 +26,7 @@ import Spinner from "../../views/elements/Spinner"; import AuthHeader from "../../views/auth/AuthHeader"; import AuthBody from "../../views/auth/AuthBody"; import { SDKContext } from "../../../contexts/SDKContext"; +import { type URLParams } from "../../../vector/url_utils.ts"; enum LoginView { Loading, @@ -43,14 +44,11 @@ const STATIC_FLOWS_TO_VIEWS: Record = { }; interface IProps { - // Query parameters from MatrixChat - realQueryParams: { - loginToken?: string; - }; - fragmentAfterLogin?: string; + urlParams: URLParams; + fragmentAfterLogin: string; // Called when the SSO login completes - onTokenLoginCompleted: () => void; + onTokenLoginCompleted: (urlParams: URLParams, fragmentAfterLogin: string) => void; } interface IState { @@ -98,8 +96,7 @@ export default class SoftLogout extends React.Component { }; private async initLogin(): Promise { - const queryParams = this.props.realQueryParams; - const hasAllParams = queryParams?.["loginToken"]; + const hasAllParams = !!this.props.urlParams?.legacy_sso; if (hasAllParams) { this.setState({ loginView: LoginView.Loading }); @@ -189,7 +186,7 @@ export default class SoftLogout extends React.Component { const isUrl = localStorage.getItem(SSO_ID_SERVER_URL_KEY) || MatrixClientPeg.safeGet().getIdentityServerUrl(); const loginType = "m.login.token"; const loginParams = { - token: this.props.realQueryParams["loginToken"], + token: this.props.urlParams?.legacy_sso?.loginToken, device_id: MatrixClientPeg.safeGet().getDeviceId() ?? undefined, }; @@ -204,9 +201,7 @@ export default class SoftLogout extends React.Component { return Lifecycle.hydrateSession(credentials) .then(() => { - if (this.props.onTokenLoginCompleted) { - this.props.onTokenLoginCompleted(); - } + this.props.onTokenLoginCompleted(this.props.urlParams, this.props.fragmentAfterLogin); return true; }) .catch((e) => { diff --git a/apps/web/src/utils/oidc/authorize.ts b/apps/web/src/utils/oidc/authorize.ts index d409396db9..b0020446ac 100644 --- a/apps/web/src/utils/oidc/authorize.ts +++ b/apps/web/src/utils/oidc/authorize.ts @@ -7,13 +7,13 @@ Please see LICENSE files in the repository root for full details. */ import { completeAuthorizationCodeGrant, generateOidcAuthorizationUrl } from "matrix-js-sdk/src/oidc/authorize"; -import { type QueryDict } from "matrix-js-sdk/src/utils"; import { type OidcClientConfig } from "matrix-js-sdk/src/matrix"; import { secureRandomString } from "matrix-js-sdk/src/randomstring"; import { type IdTokenClaims } from "oidc-client-ts"; import { OidcClientError } from "./error"; import PlatformPeg from "../../PlatformPeg"; +import { type URLParams } from "../../vector/url_utils.ts"; /** * Start OIDC authorization code flow @@ -47,22 +47,23 @@ export const startOidcLogin = async ( nonce, prompt, urlState: PlatformPeg.get()?.getOidcClientState(), + responseMode: "fragment", }); window.location.href = authorizationUrl; }; /** - * Gets `code` and `state` query params + * Gets `code` and `state` response params * - * @param queryParams + * @param urlParams - the parameters to read * @returns code and state * @throws when code and state are not valid strings */ -const getCodeAndStateFromQueryParams = (queryParams: QueryDict): { code: string; state: string } => { - const code = queryParams["code"]; - const state = queryParams["state"]; - +const getCodeAndStateFromParams = ({ + code, + state, +}: NonNullable): { code: string; state: string } => { if (!code || typeof code !== "string" || !state || typeof state !== "string") { throw new Error(OidcClientError.InvalidQueryParameters); } @@ -89,14 +90,16 @@ type CompleteOidcLoginResponse = { }; /** * Attempt to complete authorization code flow to get an access token - * @param queryParams the query-parameters extracted from the real query-string of the starting URI. + * @param urlParams the parameters extracted from the app-load URI. * @returns Promise that resolves with a CompleteOidcLoginResponse when login was successful * @throws When we failed to get a valid access token */ -export const completeOidcLogin = async (queryParams: QueryDict): Promise => { - const { code, state } = getCodeAndStateFromQueryParams(queryParams); +export const completeOidcLogin = async ( + urlParams: NonNullable, +): Promise => { + const { code, state } = getCodeAndStateFromParams(urlParams); const { homeserverUrl, tokenResponse, idTokenClaims, identityServerUrl, oidcClientSettings } = - await completeAuthorizationCodeGrant(code, state); + await completeAuthorizationCodeGrant(code, state, "fragment"); return { homeserverUrl, diff --git a/apps/web/src/vector/app.tsx b/apps/web/src/vector/app.tsx index cc4da3373a..fa31bdd732 100644 --- a/apps/web/src/vector/app.tsx +++ b/apps/web/src/vector/app.tsx @@ -17,7 +17,6 @@ import { logger } from "matrix-js-sdk/src/logger"; import { AutoDiscovery, type ClientConfig } from "matrix-js-sdk/src/matrix"; import { WrapperLifecycle, type WrapperOpts } from "@matrix-org/react-sdk-module-api/lib/lifecycles/WrapperLifecycle"; -import type { QueryDict } from "matrix-js-sdk/src/utils"; import PlatformPeg from "../PlatformPeg"; import AutoDiscoveryUtils from "../utils/AutoDiscoveryUtils"; import * as Lifecycle from "../Lifecycle"; @@ -27,8 +26,8 @@ import { SnakedObject } from "../utils/SnakedObject"; import MatrixChat from "../components/structures/MatrixChat"; import { type ValidatedServerConfig } from "../utils/ValidatedServerConfig"; import { ModuleRunner } from "../modules/ModuleRunner"; -import { parseQs } from "./url_utils"; import { getInitialScreenAfterLogin, getScreenFromLocation, init as initRouting, onNewScreen } from "./routing"; +import { type URLParams } from "./url_utils.ts"; import { UserFriendlyError } from "../languageHandler"; import { ModuleApi } from "../modules/Api"; import { RoomView } from "../components/structures/RoomView"; @@ -41,20 +40,22 @@ logger.log(`Application is running in ${process.env.NODE_ENV} mode`); window.matrixLogger = logger; -function onTokenLoginCompleted(): void { - // if we did a token login, we're now left with the token, hs and is - // url as query params in the url; - // if we did an oidc authorization code flow login, we're left with the auth code and state - // as query params in the url; - // a little nasty but let's redirect to clear them. +function onTokenLoginCompleted(urlParams: URLParams, fragmentAfterLogin: string): void { const url = new URL(window.location.href); - url.searchParams.delete("no_universal_links"); - url.searchParams.delete("loginToken"); - url.searchParams.delete("state"); - url.searchParams.delete("code"); + // if we did a token login, we're now left with the login token as query param in the url; clear it out + for (const param in { ...urlParams.legacy_sso }) { + url.searchParams.delete(param); + } - logger.log(`Redirecting to ${url.href} to drop delegated authentication params from queryparams`); + // Added by OIDC auth to avoid being hijacked by Element X on macOS + url.searchParams.delete("no_universal_links"); + + // if we did an oidc authorization code flow login, we're left with the auth code and state in the fragment in the url, + // we clear it out by using the fragmentAfterLogin + url.hash = fragmentAfterLogin; + + logger.log(`Redirecting to ${url.href} to drop authentication params from url`); window.history.replaceState(null, "", url.href); } @@ -87,7 +88,7 @@ async function redirectToSso(config: ValidatedServerConfig): Promise { return false; } -export async function loadApp(fragParams: QueryDict, matrixChatRef: React.Ref): Promise { +export async function loadApp(urlParams: URLParams, matrixChatRef: React.Ref): Promise { // XXX: This lives here because certain components import so many things that importing it in a sensible place (eg. // the builtins module or init.tsx) causes a circular dependency. ModuleApi.instance.builtins.setComponents({ @@ -99,8 +100,6 @@ export async function loadApp(fragParams: QueryDict, matrixChatRef: React.Ref { // give rageshake a chance to load/fail, we don't actually assert rageshake loads, we allow it to fail if no IDB await settled(rageshakePromise); - const fragparts = parseQsFromFragment(window.location); + const parsedUrl = parseAppUrl(window.location); // don't try to redirect to the native apps if we're // verifying a 3pid (but after we've loaded the config) // or if the user is following a deep link // (https://github.com/element-hq/element-web/issues/7378) - const preventRedirect = fragparts.params.client_secret || fragparts.location.length > 0; + const preventRedirect = !!parsedUrl.params.threepid || parsedUrl.location.length > 0; if (!preventRedirect) { const isIos = /iPad|iPhone|iPod/.test(navigator.userAgent) && !window.MSStream; @@ -232,7 +232,7 @@ async function start(): Promise { // Finally, load the app. All of the other react-sdk imports are in this file which causes the skinner to // run on the components. - await loadApp(fragparts.params); + await loadApp(parsedUrl.params); } catch (err) { logger.error(err); // Like the compatibility page, AWOOOOOGA at the user diff --git a/apps/web/src/vector/init.tsx b/apps/web/src/vector/init.tsx index 6706b75672..2557e74ba3 100644 --- a/apps/web/src/vector/init.tsx +++ b/apps/web/src/vector/init.tsx @@ -13,7 +13,6 @@ import React, { StrictMode } from "react"; import { logger } from "matrix-js-sdk/src/logger"; import { ModuleLoader } from "@element-hq/element-web-module-api"; -import type { QueryDict } from "matrix-js-sdk/src/utils"; import * as languageHandler from "../languageHandler"; import SettingsStore from "../settings/SettingsStore"; import PlatformPeg from "../PlatformPeg"; @@ -26,6 +25,7 @@ import PWAPlatform from "./platform/PWAPlatform"; import WebPlatform from "./platform/WebPlatform"; import { initRageshake, initRageshakeStore } from "./rageshakesetup"; import { ModuleApi } from "../modules/Api.ts"; +import { type URLParams } from "./url_utils.ts"; export const rageshakePromise = initRageshake(); @@ -86,7 +86,7 @@ export async function loadTheme(): Promise { return setTheme(); } -export async function loadApp(fragParams: QueryDict): Promise { +export async function loadApp(urlParams: URLParams): Promise { // load app.js async so that its code is not executed immediately and we can catch any exceptions const module = await import( /* webpackChunkName: "element-web-app" */ @@ -96,7 +96,7 @@ export async function loadApp(fragParams: QueryDict): Promise { function setWindowMatrixChat(matrixChat: MatrixChat): void { window.matrixChat = matrixChat; } - const app = await module.loadApp(fragParams, setWindowMatrixChat); + const app = await module.loadApp(urlParams, setWindowMatrixChat); const root = createRoot(document.getElementById("matrixchat")!); root.render(app); } diff --git a/apps/web/src/vector/platform/WebPlatform.ts b/apps/web/src/vector/platform/WebPlatform.ts index a794e160a4..88aa7ee9cc 100644 --- a/apps/web/src/vector/platform/WebPlatform.ts +++ b/apps/web/src/vector/platform/WebPlatform.ts @@ -16,7 +16,6 @@ import dis from "../../dispatcher/dispatcher"; import { hideToast as hideUpdateToast, showToast as showUpdateToast } from "../../toasts/UpdateToast"; import { Action } from "../../dispatcher/actions"; import { type CheckUpdatesPayload } from "../../dispatcher/payloads/CheckUpdatesPayload"; -import { parseQs } from "../url_utils"; import { _t } from "../../languageHandler"; import ToastStore from "../../stores/ToastStore.ts"; import GenericToast from "../../components/views/toasts/GenericToast.tsx"; @@ -174,8 +173,8 @@ export default class WebPlatform extends BasePlatform { // cache-control: nocache HTTP header set, but Firefox doesn't always obey it :/ console.log("startUpdater, current version is " + getNormalizedAppVersion(WebPlatform.VERSION)); void this.pollForUpdate((version: string, newVersion: string) => { - const query = parseQs(location); - if (query.updated) { + const url = new URL(window.location.href); + if (url.searchParams.has("updated")) { console.log("Update reloaded but still on an old version, stopping"); // We just reloaded already and are still on the old version! // Show the toast rather than reload in a loop. @@ -184,7 +183,6 @@ export default class WebPlatform extends BasePlatform { } // Set updated as a cachebusting query param and reload the page. - const url = new URL(window.location.href); url.searchParams.set("updated", newVersion); console.log("Update reloading to " + url.toString()); window.location.href = url.toString(); diff --git a/apps/web/src/vector/routing.ts b/apps/web/src/vector/routing.ts index 964c0c0684..149316424c 100644 --- a/apps/web/src/vector/routing.ts +++ b/apps/web/src/vector/routing.ts @@ -11,15 +11,20 @@ Please see LICENSE files in the repository root for full details. import { logger } from "matrix-js-sdk/src/logger"; import { type QueryDict } from "matrix-js-sdk/src/utils"; -import { parseQsFromFragment } from "./url_utils"; +import { parseQsFromFragment, searchParamsToQueryDict } from "./url_utils"; let lastLocationHashSet: string | null = null; -export function getScreenFromLocation(location: Location): { screen: string; params: QueryDict } { +export interface IScreen { + screen: string; + params: QueryDict; +} + +export function getScreenFromLocation(location: Location): IScreen { const fragparts = parseQsFromFragment(location); return { screen: fragparts.location.substring(1), - params: fragparts.params, + params: fragparts.params ? searchParamsToQueryDict(fragparts.params) : {}, }; } diff --git a/apps/web/src/vector/url_utils.ts b/apps/web/src/vector/url_utils.ts index 2b5202806e..3ea6166316 100644 --- a/apps/web/src/vector/url_utils.ts +++ b/apps/web/src/vector/url_utils.ts @@ -5,32 +5,129 @@ SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only OR LicenseRef-Element-Com Please see LICENSE files in the repository root for full details. */ -import { type QueryDict, decodeParams } from "matrix-js-sdk/src/utils"; +import { type QueryDict } from "matrix-js-sdk/src/utils"; // We want to support some name / value pairs in the fragment -// so we're re-using query string like format -// -export function parseQsFromFragment(location: Location): { location: string; params: QueryDict } { +// so we're re-using query string like format, where we accept a `?key=value&key2=value2` string at the end of the hash +// but we also accept a hash like `key=value&key2=value2` for compatibility with oAuth response_mode = fragment +export function parseQsFromFragment(url: Location | URL): { location: string; params?: URLSearchParams } { // if we have a fragment, it will start with '#', which we need to drop. // (if we don't, this will return ''). - const fragment = location.hash.substring(1); + const fragment = url.hash.substring(1); // our fragment may contain a query-param-like section. we need to fish // this out *before* URI-decoding because the params may contain ? and & // characters which are only URI-encoded once. - const hashparts = fragment.split("?"); + const [main, query] = fragment.split("?", 2); - const result = { - location: decodeURIComponent(hashparts[0]), - params: {}, - }; - - if (hashparts.length > 1) { - result.params = decodeParams(hashparts[1]); + // Handle oAuth-style fragment parameters + if (main.includes("=")) { + return { + location: "", + params: new URLSearchParams(main), + }; } - return result; + + return { + location: decodeURIComponent(main), + params: query ? new URLSearchParams(query) : undefined, + }; } -export function parseQs(location: Location): QueryDict { - return decodeParams(location.search.substring(1)); +/** + * Convert a URLSearchParams object to QueryDict + * Any keys with multiple values will be grouped into an array + * @param params the URLSearchParams to convert + */ +export function searchParamsToQueryDict(params: URLSearchParams): QueryDict { + const queryDict: QueryDict = {}; + for (const key of params.keys()) { + const val = params.getAll(key); + queryDict[key] = val.length === 1 ? val[0] : val; + } + return queryDict; +} + +const urlParameterConfig = { + // Query string params for legacy SSO login, added by the Matrix homeserver + legacy_sso: { + keys: ["loginToken"], + location: "query", + }, + // Fragment params for OIDC login, added by the Identity Provider + oidc: { + keys: ["code", "state"], + location: "fragment", + }, + // Fragment params relating to 3pid (email) invites, added in url within the invite email itself + threepid: { + keys: ["client_secret", "session_id", "hs_url", "is_url", "sid"], + location: "fragment", + }, + // XXX: unclear where, if anywhere, this is set + defaults: { + keys: ["defaultUsername"], + location: "fragment", + }, + // XXX: Fragment params seemingly relating to 3pid invites, though the code in the area doubts they are ever specified + guest: { + keys: ["guest_user_id", "guest_access_token"], + location: "fragment", + }, +} as const satisfies Record< + string, + { + keys: string[]; + // Query params live in the query string, in the middle of the URL, after a `?`, in a `key=value` format, delimited by `&`. + // Fragment params live in the fragment string, at the end of the URL, after a `?`, in a `key=value` format, delimited by `&`. + location: "query" | "fragment"; + } +>; + +export type URLParams = Partial<{ + -readonly [K in keyof typeof urlParameterConfig]: Partial<{ + [P in (typeof urlParameterConfig)[K]["keys"][number]]: string; + }>; +}>; + +/** + * Utility to parse parameters held in the app's URL. + * Currently focusing only on at-load URL parameters. + * @param url - the URL to parse. + * @return an object keyed by the groups defined in {@link urlParameterConfig} with values for each key listed, + * sourced from the location (query/fragment/either) specified. If no parameters in a group are found the entire group + * will be omitted from the returned object to simplify presence checking. + */ +export function parseAppUrl(url: Location | URL): { + location: string; + params: URLParams; +} { + const queryParams = new URLSearchParams(url.search); + const parsedFragment = parseQsFromFragment(url); + + const urlParams: Partial = {}; + + for (const group in urlParameterConfig) { + const groupKey = group as keyof URLParams; + const groupConfig = urlParameterConfig[groupKey]; + + const params = groupConfig.location === "fragment" ? parsedFragment.params : queryParams; + if (!params) continue; // no params + + const target: Record = {}; + for (const k of groupConfig.keys) { + const key = k as (typeof groupConfig)["keys"][number]; + + const value = params.get(key); + if (value !== null) { + target[key] = value; + } + } + + if (Object.keys(target).length > 0) { + urlParams[groupKey] = target; + } + } + + return { params: urlParams as URLParams, location: parsedFragment.location }; } diff --git a/apps/web/test/unit-tests/Lifecycle-test.ts b/apps/web/test/unit-tests/Lifecycle-test.ts index a9336db114..adac08989d 100644 --- a/apps/web/test/unit-tests/Lifecycle-test.ts +++ b/apps/web/test/unit-tests/Lifecycle-test.ts @@ -169,7 +169,7 @@ describe("Lifecycle", () => { const prom = Lifecycle.loadSession({ enableGuest: true, guestHsUrl: "https://guest.server", - fragmentQueryParams: { guest_user_id: "a", guest_access_token: "b" }, + urlParams: { guest: { guest_user_id: "a", guest_access_token: "b" } }, abortSignal: abortController.signal, }); abortController.abort(); diff --git a/apps/web/test/unit-tests/components/structures/MatrixChat-test.tsx b/apps/web/test/unit-tests/components/structures/MatrixChat-test.tsx index ac5c2bad9f..4e60da8d2f 100644 --- a/apps/web/test/unit-tests/components/structures/MatrixChat-test.tsx +++ b/apps/web/test/unit-tests/components/structures/MatrixChat-test.tsx @@ -223,7 +223,7 @@ describe("", () => { }, onNewScreen: jest.fn(), onTokenLoginCompleted: jest.fn(), - realQueryParams: {}, + urlParams: {}, }; mockClient = getMockClientWithEventEmitter(getMockClientMethods()); @@ -320,9 +320,11 @@ describe("", () => { const code = "test-oidc-auth-code"; const state = "test-oidc-state"; - const realQueryParams = { - code, - state: state, + const urlParams = { + oidc: { + code, + state: state, + }, }; const deviceId = "test-device-id"; @@ -383,11 +385,13 @@ describe("", () => { }); it("should fail when query params do not include valid code and state", async () => { - const queryParams = { - code: 123, - state: "abc", + const urlParams = { + oidc: { + code: "", + state: "abc", + }, }; - getComponent({ realQueryParams: queryParams }); + getComponent({ urlParams }); await flushPromises(); @@ -400,15 +404,15 @@ describe("", () => { }); it("should make correct request to complete authorization", async () => { - getComponent({ realQueryParams }); + getComponent({ urlParams }); await flushPromises(); - expect(completeAuthorizationCodeGrant).toHaveBeenCalledWith(code, state); + expect(completeAuthorizationCodeGrant).toHaveBeenCalledWith(code, state, "fragment"); }); it("should look up userId using access token", async () => { - getComponent({ realQueryParams }); + getComponent({ urlParams }); await flushPromises(); @@ -423,7 +427,7 @@ describe("", () => { it("should log error and return to welcome page when userId lookup fails", async () => { loginClient.whoami.mockRejectedValue(new Error("oups")); - getComponent({ realQueryParams }); + getComponent({ urlParams }); await flushPromises(); @@ -436,7 +440,7 @@ describe("", () => { it("should call onTokenLoginCompleted", async () => { const onTokenLoginCompleted = jest.fn(); - getComponent({ realQueryParams, onTokenLoginCompleted }); + getComponent({ urlParams, onTokenLoginCompleted }); await waitFor(() => expect(onTokenLoginCompleted).toHaveBeenCalled()); }); @@ -450,7 +454,7 @@ describe("", () => { mocked(completeAuthorizationCodeGrant).mockRejectedValue( new Error(OidcError.MissingOrInvalidStoredState), ); - getComponent({ realQueryParams }); + getComponent({ urlParams }); await flushPromises(); @@ -465,7 +469,7 @@ describe("", () => { }); it("should log and return to welcome page", async () => { - getComponent({ realQueryParams }); + getComponent({ urlParams }); await flushPromises(); @@ -479,7 +483,7 @@ describe("", () => { }); it("should not clear storage", async () => { - getComponent({ realQueryParams }); + getComponent({ urlParams }); await flushPromises(); @@ -488,7 +492,7 @@ describe("", () => { it("should not store clientId or issuer", async () => { const sessionStorageSetSpy = jest.spyOn(sessionStorage.__proto__, "setItem"); - getComponent({ realQueryParams }); + getComponent({ urlParams }); await flushPromises(); @@ -509,7 +513,7 @@ describe("", () => { }); it("should persist login credentials", async () => { - getComponent({ realQueryParams }); + getComponent({ urlParams }); await waitFor(() => expect(localStorage.getItem("mx_device_id")).toEqual(deviceId)); expect(localStorage.getItem("mx_hs_url")).toEqual(homeserverUrl); @@ -518,14 +522,14 @@ describe("", () => { }); it("should store clientId and issuer in session storage", async () => { - getComponent({ realQueryParams }); + getComponent({ urlParams }); await waitFor(() => expect(localStorage.getItem("mx_oidc_client_id")).toEqual(clientId)); await waitFor(() => expect(localStorage.getItem("mx_oidc_token_issuer")).toEqual(issuer)); }); it("should set logged in and start MatrixClient", async () => { - getComponent({ realQueryParams }); + getComponent({ urlParams }); defaultDispatcher.dispatch({ action: Action.WillStartClient, @@ -545,7 +549,7 @@ describe("", () => { jest.spyOn(Lifecycle, "attemptDelegatedAuthLogin"); - getComponent({ realQueryParams }); + getComponent({ urlParams }); await flushPromises(); expect(Lifecycle.attemptDelegatedAuthLogin).toHaveBeenCalled(); @@ -559,7 +563,7 @@ describe("", () => { jest.spyOn(Lifecycle, "attemptDelegatedAuthLogin"); - getComponent({ realQueryParams }); + getComponent({ urlParams }); await flushPromises(); expect(Lifecycle.attemptDelegatedAuthLogin).toHaveBeenCalled(); @@ -1120,8 +1124,10 @@ describe("", () => { describe("when query params have a loginToken", () => { const loginToken = "test-login-token"; - const realQueryParams = { - loginToken, + const urlParams = { + legacy_sso: { + loginToken, + }, }; let loginClient!: ReturnType; @@ -1150,7 +1156,7 @@ describe("", () => { mocked(loginClient.getCrypto()!.userHasCrossSigningKeys).mockResolvedValue(true); // When we load the page - getComponent({ realQueryParams }); + getComponent({ urlParams }); defaultDispatcher.dispatch({ action: Action.WillStartClient, @@ -1400,8 +1406,10 @@ describe("", () => { describe("when query params have a loginToken", () => { const loginToken = "test-login-token"; - const realQueryParams = { - loginToken, + const urlParams = { + legacy_sso: { + loginToken, + }, }; let loginClient!: ReturnType; @@ -1426,7 +1434,7 @@ describe("", () => { it("should show an error dialog when no homeserver is found in local storage", async () => { localStorage.removeItem("mx_sso_hs_url"); const localStorageGetSpy = jest.spyOn(localStorage.__proto__, "getItem"); - getComponent({ realQueryParams }); + getComponent({ urlParams }); await flushPromises(); expect(localStorageGetSpy).toHaveBeenCalledWith("mx_sso_hs_url"); @@ -1444,7 +1452,7 @@ describe("", () => { }); it("should attempt token login", async () => { - getComponent({ realQueryParams }); + getComponent({ urlParams }); await flushPromises(); expect(loginClient.login).toHaveBeenCalledWith("m.login.token", { @@ -1455,7 +1463,7 @@ describe("", () => { it("should call onTokenLoginCompleted", async () => { const onTokenLoginCompleted = jest.fn(); - getComponent({ realQueryParams, onTokenLoginCompleted }); + getComponent({ urlParams, onTokenLoginCompleted }); await waitFor(() => expect(onTokenLoginCompleted).toHaveBeenCalled()); }); @@ -1465,7 +1473,7 @@ describe("", () => { loginClient.login.mockRejectedValue(new Error("oups")); }); it("should show a dialog", async () => { - getComponent({ realQueryParams }); + getComponent({ urlParams }); await flushPromises(); @@ -1480,7 +1488,7 @@ describe("", () => { }); it("should not clear storage", async () => { - getComponent({ realQueryParams }); + getComponent({ urlParams }); await flushPromises(); @@ -1502,7 +1510,7 @@ describe("", () => { it("should clear storage", async () => { const localStorageClearSpy = jest.spyOn(localStorage.__proto__, "clear"); - getComponent({ realQueryParams }); + getComponent({ urlParams }); // just check we called the clearStorage function await waitFor(() => expect(loginClient.clearStores).toHaveBeenCalled()); @@ -1511,7 +1519,7 @@ describe("", () => { }); it("should persist login credentials", async () => { - getComponent({ realQueryParams }); + getComponent({ urlParams }); await waitFor(() => expect(localStorage.getItem("mx_hs_url")).toEqual(serverConfig.hsUrl)); expect(localStorage.getItem("mx_user_id")).toEqual(userId); @@ -1521,7 +1529,7 @@ describe("", () => { it("should set fresh login flag in session storage", async () => { const sessionStorageSetSpy = jest.spyOn(sessionStorage.__proto__, "setItem"); - getComponent({ realQueryParams }); + getComponent({ urlParams }); await waitFor(() => expect(sessionStorageSetSpy).toHaveBeenCalledWith("mx_fresh_login", "true")); }); @@ -1537,13 +1545,13 @@ describe("", () => { }, }; loginClient.login.mockResolvedValue(loginResponseWithWellKnown); - getComponent({ realQueryParams }); + getComponent({ urlParams }); await waitFor(() => expect(localStorage.getItem("mx_hs_url")).toEqual(hsUrlFromWk)); }); it("should continue to post login setup when no session is found in local storage", async () => { - getComponent({ realQueryParams }); + getComponent({ urlParams }); defaultDispatcher.dispatch({ action: Action.WillStartClient, }); @@ -1589,6 +1597,7 @@ describe("", () => { getComponent({ initialScreenAfterLogin: { screen: "start_sso", + params: {}, }, }); @@ -1604,6 +1613,7 @@ describe("", () => { getComponent({ initialScreenAfterLogin: { screen: "start_cas", + params: {}, }, }); diff --git a/apps/web/test/unit-tests/utils/oidc/authorize-test.ts b/apps/web/test/unit-tests/utils/oidc/authorize-test.ts index d23157e097..c28e6325d7 100644 --- a/apps/web/test/unit-tests/utils/oidc/authorize-test.ts +++ b/apps/web/test/unit-tests/utils/oidc/authorize-test.ts @@ -75,7 +75,7 @@ describe("OIDC authorization", () => { const authUrl = new URL(window.location.href); - expect(authUrl.searchParams.get("response_mode")).toEqual("query"); + expect(authUrl.searchParams.get("response_mode")).toEqual("fragment"); expect(authUrl.searchParams.get("response_type")).toEqual("code"); expect(authUrl.searchParams.get("client_id")).toEqual(clientId); expect(authUrl.searchParams.get("code_challenge_method")).toEqual("S256"); @@ -95,7 +95,7 @@ describe("OIDC authorization", () => { describe("completeOidcLogin()", () => { const state = "test-state-444"; const code = "test-code-777"; - const queryDict = { + const params = { code, state: state, }; @@ -137,13 +137,13 @@ describe("OIDC authorization", () => { }); it("should make request complete authorization code grant", async () => { - await completeOidcLogin(queryDict); + await completeOidcLogin(params); - expect(completeAuthorizationCodeGrant).toHaveBeenCalledWith(code, state); + expect(completeAuthorizationCodeGrant).toHaveBeenCalledWith(code, state, "fragment"); }); it("should return accessToken, configured homeserver and identityServer", async () => { - const result = await completeOidcLogin(queryDict); + const result = await completeOidcLogin(params); expect(result).toEqual({ accessToken: tokenResponse.access_token, diff --git a/apps/web/test/unit-tests/vector/app-test.ts b/apps/web/test/unit-tests/vector/app-test.ts index c2e9eb1cff..381f4a2c53 100644 --- a/apps/web/test/unit-tests/vector/app-test.ts +++ b/apps/web/test/unit-tests/vector/app-test.ts @@ -85,7 +85,7 @@ describe("sso_redirect_options", () => { await loadApp({}, jest.fn()); expect(startOidcLoginSpy).toHaveBeenCalledWith( - "https://auth.org/auth?client_id=12345&redirect_uri=https%3A%2F%2Fapp.element.io%2F%3Fno_universal_links%3Dtrue&response_type=code&scope=openid+urn%3Amatrix%3Aorg.matrix.msc2967.client%3Aapi%3A*+urn%3Amatrix%3Aorg.matrix.msc2967.client%3Adevice%3AwKpa6hpi3Y&nonce=38QgU2Pomx&state=10000000100040008000100000000000&code_challenge=awE81eIsGff70JahvrTqWRbGKLI10ooyo_Xm1sxuZvU&code_challenge_method=S256&response_mode=query", + "https://auth.org/auth?client_id=12345&redirect_uri=https%3A%2F%2Fapp.element.io%2F%3Fno_universal_links%3Dtrue&response_type=code&scope=openid+urn%3Amatrix%3Aorg.matrix.msc2967.client%3Aapi%3A*+urn%3Amatrix%3Aorg.matrix.msc2967.client%3Adevice%3AwKpa6hpi3Y&nonce=38QgU2Pomx&state=10000000100040008000100000000000&code_challenge=awE81eIsGff70JahvrTqWRbGKLI10ooyo_Xm1sxuZvU&code_challenge_method=S256&response_mode=fragment", ); }); }); diff --git a/apps/web/test/unit-tests/vector/init-test.ts b/apps/web/test/unit-tests/vector/init-test.ts index 4c9c2a529c..9c39ba652a 100644 --- a/apps/web/test/unit-tests/vector/init-test.ts +++ b/apps/web/test/unit-tests/vector/init-test.ts @@ -1,6 +1,6 @@ /** * @jest-environment jest-fixed-jsdom - * @jest-environment-options {"url": "https://app.element.io/?loginToken=123&state=abc&code=xyz&no_universal_links&something_else=value"} + * @jest-environment-options {"url": "https://app.element.io/?loginToken=123&no_universal_links&something_else=value#/home?state=abc&code=xyz"} */ /* @@ -16,6 +16,7 @@ import { waitFor, screen } from "jest-matrix-react"; import { loadApp, showError, showIncompatibleBrowser } from "../../../src/vector/init.tsx"; import SdkConfig from "../../../src/SdkConfig.ts"; import MatrixChat from "../../../src/components/structures/MatrixChat.tsx"; +import { parseAppUrl } from "../../../src/vector/url_utils.ts"; function setUpMatrixChatDiv() { document.getElementById("matrixchat")?.remove(); @@ -57,17 +58,13 @@ describe("loadApp", () => { await waitFor(() => expect(window.matrixChat).toBeInstanceOf(MatrixChat)); }); - it("should pass onTokenLoginCompleted which strips searchParams to MatrixChat", async () => { + it("should pass onTokenLoginCompleted which strips searchParams & fragment to MatrixChat", async () => { const spy = jest.spyOn(window.history, "replaceState"); await loadApp({}); await waitFor(() => expect(window.matrixChat).toBeInstanceOf(MatrixChat)); - window.matrixChat!.props.onTokenLoginCompleted(); + window.matrixChat!.props.onTokenLoginCompleted(parseAppUrl(window.location).params, "/home"); - expect(spy).toHaveBeenCalledWith( - null, - "", - expect.stringContaining("https://app.element.io/?something_else=value"), - ); + expect(spy).toHaveBeenCalledWith(null, "", "https://app.element.io/?something_else=value#/home"); }); }); diff --git a/apps/web/test/unit-tests/vector/url_utils-test.ts b/apps/web/test/unit-tests/vector/url_utils-test.ts index 242ca9ace9..f6f42f573c 100644 --- a/apps/web/test/unit-tests/vector/url_utils-test.ts +++ b/apps/web/test/unit-tests/vector/url_utils-test.ts @@ -5,37 +5,54 @@ SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only OR LicenseRef-Element-Com Please see LICENSE files in the repository root for full details. */ -import { parseQsFromFragment, parseQs } from "../../../src/vector/url_utils"; +import { parseAppUrl, parseQsFromFragment, searchParamsToQueryDict } from "../../../src/vector/url_utils"; -describe("url_utils.ts", function () { - // @ts-ignore - const location: Location = { - hash: "", - search: "", - }; +// @ts-ignore +const location: Location = { + hash: "", + search: "", +}; - it("parseQsFromFragment", function () { - location.hash = "/home?foo=bar"; +describe("parseQsFromFragment", () => { + it("should parse correctly", () => { + location.hash = "#/home?foo=bar"; expect(parseQsFromFragment(location)).toEqual({ - location: "home", - params: { + location: "/home", + params: new URLSearchParams({ foo: "bar", - }, - }); - }); - - it("parseQs", function () { - location.search = "?foo=bar"; - expect(parseQs(location)).toEqual({ - foo: "bar", - }); - }); - - it("parseQs with arrays", function () { - location.search = "?via=s1&via=s2&via=s2&foo=bar"; - expect(parseQs(location)).toEqual({ - via: ["s1", "s2", "s2"], - foo: "bar", + }), }); }); }); + +describe("searchParamsToQueryDict", () => { + it("should handle arrays correctly", () => { + const u = new URLSearchParams("a=b&b=c&c=d&a=e&a=f"); + expect(searchParamsToQueryDict(u)).toEqual({ + a: ["b", "e", "f"], + b: "c", + c: "d", + }); + }); +}); + +describe("parseUrlParameters", () => { + it("should parse legacy sso parameters from query", () => { + const u = new URL("https://app.element.io?loginToken=foobar"); + const parsed = parseAppUrl(u); + expect(parsed.params.legacy_sso?.loginToken).toEqual("foobar"); + }); + + it("should parse oidc parameters from oauth-fragment", () => { + const u = new URL("https://app.element.io/#code=foobar&state=barfoo"); + const parsed = parseAppUrl(u); + expect(parsed.params.oidc?.code).toEqual("foobar"); + expect(parsed.params.oidc?.state).toEqual("barfoo"); + }); + + it("should parse guest parameters", () => { + const u = new URL("https://app.element.io?foo=bar#/room/!roomId:server?guest_access_token=foobar"); + const parsed = parseAppUrl(u); + expect(parsed.params.guest?.guest_access_token).toEqual("foobar"); + }); +});