Fall back to OIDC response_mode query if fragment unsupported (#33169)

* Fall back to OIDC response_mode query if fragment unsupported

* Tidy comments

* Fix test

Author: t3chguy

apps/web/src/Lifecycle.ts

@@ -277,9 +277,10 @@ export async function attemptDelegatedAuthLogin(
     defaultDeviceDisplayName?: string,
     fragmentAfterLogin?: string,
 ): Promise<boolean> {
-    if (urlParams.oidc) {
-        console.log("We have OIDC params - attempting OIDC login");
-        return attemptOidcNativeLogin(urlParams["oidc"]);
+    if (urlParams.oidc_fragment) {
+        return attemptOidcNativeLogin(urlParams.oidc_fragment, "fragment");
+    } else if (urlParams.oidc_query) {
+        return attemptOidcNativeLogin(urlParams.oidc_query, "query");
     }
 
     return attemptTokenLogin(urlParams["legacy_sso"], defaultDeviceDisplayName, fragmentAfterLogin);
@@ -288,12 +289,18 @@ export async function attemptDelegatedAuthLogin(
 /**
  * Attempt to login by completing OIDC authorization code flow
  * @param urlParams subset of app-load url parameters relating to oidc auth
+ * @param responseMode - the response_mode used in the auth request
  * @returns Promise that resolves to true when login succeeded, else false
  */
-async function attemptOidcNativeLogin(urlParams: NonNullable<URLParams["oidc"]>): Promise<boolean> {
+async function attemptOidcNativeLogin(
+    urlParams: NonNullable<URLParams["oidc_fragment"]>,
+    responseMode: "fragment" | "query",
+): Promise<boolean> {
+    console.log("We have OIDC params - attempting OIDC login");
+
     try {
         const { accessToken, refreshToken, homeserverUrl, identityServerUrl, idToken, clientId, issuer } =
-            await completeOidcLogin(urlParams);
+            await completeOidcLogin(urlParams, responseMode);
 
         const {
             user_id: userId,
@@ -1036,7 +1043,7 @@ export function isLoggingOut(): boolean {
  * By the time this method is called, we have successfully logged in if necessary, and the client has been set up with
  * the access token.
  *
- * Emits {@link Acction.WillStartClient} before starting the client, and {@link Action.ClientStarted} when the client has
+ * Emits {@link Action.WillStartClient} before starting the client, and {@link Action.ClientStarted} when the client has
  * been started.
  *
  * @param client the matrix client to start

apps/web/src/components/structures/MatrixChat.tsx

@@ -350,7 +350,11 @@ export default class MatrixChat extends React.PureComponent<IProps, IState> {
         );
 
         // remove the loginToken or auth code from the URL regardless
-        if (!!this.props.urlParams.legacy_sso || !!this.props.urlParams.oidc) {
+        if (
+            !!this.props.urlParams.legacy_sso ||
+            !!this.props.urlParams.oidc_fragment ||
+            !!this.props.urlParams.oidc_query
+        ) {
             this.props.onTokenLoginCompleted(this.props.urlParams, this.getFragmentAfterLogin());
         }
 
@@ -408,7 +412,7 @@ export default class MatrixChat extends React.PureComponent<IProps, IState> {
      * {@link onWillStartClient} and {@link onClientStarted} will already have been called (but not necessarily
      * completed).
      *
-     * This method either calls {@link onLiggedIn} directly, or switches to {@link Views.E2E_SETUP} or
+     * This method either calls {@link onLoggedIn} directly, or switches to {@link Views.E2E_SETUP} or
      * {@link Views.COMPLETE_SECURITY}, which will later call {@link onCompleteSecurityE2eSetupFinished}.
      */
     private async postLoginSetup(): Promise<void> {

apps/web/src/utils/oidc/authorize.ts

@@ -23,6 +23,7 @@ import { type URLParams } from "../../vector/url_utils.ts";
  * @param clientId this client's id as registered with configured issuer
  * @param homeserverUrl target homeserver
  * @param identityServerUrl OPTIONAL target identity server
+ * @param isRegistration if true will set the prompt to "create"
  * @returns Promise that resolves after we have navigated to auth endpoint
  */
 export const startOidcLogin = async (
@@ -47,7 +48,7 @@ export const startOidcLogin = async (
         nonce,
         prompt,
         urlState: PlatformPeg.get()?.getOidcClientState(),
-        responseMode: "fragment",
+        responseMode: delegatedAuthConfig.response_modes_supported?.includes("fragment") ? "fragment" : "query",
     });
 
     window.location.href = authorizationUrl;
@@ -57,15 +58,20 @@ export const startOidcLogin = async (
  * Gets `code` and `state` response params
  *
  * @param urlParams - the parameters to read
+ * @param responseMode - the response_mode used in the auth request
  * @returns code and state
  * @throws when code and state are not valid strings
  */
-const getCodeAndStateFromParams = ({
-    code,
-    state,
-}: NonNullable<URLParams["oidc"]>): { code: string; state: string } => {
+const getCodeAndStateFromParams = (
+    { code, state }: NonNullable<URLParams["oidc_fragment"]>,
+    responseMode: "fragment" | "query",
+): { code: string; state: string } => {
     if (!code || typeof code !== "string" || !state || typeof state !== "string") {
-        throw new Error(OidcClientError.InvalidQueryParameters);
+        if (responseMode === "fragment") {
+            throw new Error(OidcClientError.InvalidFragmentParameters);
+        } else {
+            throw new Error(OidcClientError.InvalidQueryParameters);
+        }
     }
     return { code, state };
 };
@@ -91,15 +97,17 @@ type CompleteOidcLoginResponse = {
 /**
  * Attempt to complete authorization code flow to get an access token
  * @param urlParams the parameters extracted from the app-load URI.
+ * @param responseMode - the response_mode used in the auth request
  * @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 (
-    urlParams: NonNullable<URLParams["oidc"]>,
+    urlParams: NonNullable<URLParams["oidc_fragment"]>,
+    responseMode: "fragment" | "query",
 ): Promise<CompleteOidcLoginResponse> => {
-    const { code, state } = getCodeAndStateFromParams(urlParams);
+    const { code, state } = getCodeAndStateFromParams(urlParams, responseMode);
     const { homeserverUrl, tokenResponse, idTokenClaims, identityServerUrl, oidcClientSettings } =
-        await completeAuthorizationCodeGrant(code, state, "fragment");
+        await completeAuthorizationCodeGrant(code, state, responseMode);
 
     return {
         homeserverUrl,

apps/web/src/utils/oidc/error.ts

@@ -17,6 +17,7 @@ import { _t } from "../../languageHandler";
  */
 export enum OidcClientError {
     InvalidQueryParameters = "Invalid query parameters for OIDC native login. `code` and `state` are required.",
+    InvalidFragmentParameters = "Invalid fragment parameters for OIDC native login. `code` and `state` are required.",
 }
 
 /**
@@ -30,6 +31,7 @@ export const getOidcErrorMessage = (error: Error): string | ReactNode => {
         case OidcError.MissingOrInvalidStoredState:
             return _t("auth|oidc|missing_or_invalid_stored_state");
         case OidcClientError.InvalidQueryParameters:
+        case OidcClientError.InvalidFragmentParameters:
         case OidcError.CodeExchangeFailed:
         case OidcError.InvalidBearerTokenResponse:
         case OidcError.InvalidIdToken:

apps/web/src/vector/app.tsx

@@ -44,7 +44,7 @@ function onTokenLoginCompleted(urlParams: URLParams, fragmentAfterLogin: string)
     const url = new URL(window.location.href);
 
     // 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 }) {
+    for (const param in { ...urlParams.legacy_sso, ...urlParams.oidc_query }) {
         url.searchParams.delete(param);
     }
 
@@ -112,7 +112,7 @@ export async function loadApp(urlParams: URLParams, matrixChatRef: React.Ref<Mat
     // Before we continue, let's see if we're supposed to do an SSO redirect
     const [userId] = await Lifecycle.getStoredSessionOwner();
     const hasPossibleToken = !!userId;
-    const isReturningFromSso = !!urlParams.legacy_sso || !!urlParams.oidc;
+    const isReturningFromSso = !!urlParams.legacy_sso || !!urlParams.oidc_fragment || !!urlParams.oidc_query;
     const ssoRedirects = config.sso_redirect_options || {};
     let autoRedirect = ssoRedirects.immediate === true;
     // XXX: This path matching is a bit brittle, but better to do it early instead of in the app code.

apps/web/src/vector/url_utils.ts

@@ -55,10 +55,15 @@ const urlParameterConfig = {
         location: "query",
     },
     // Fragment params for OIDC login, added by the Identity Provider
-    oidc: {
+    oidc_fragment: {
         keys: ["code", "state"],
         location: "fragment",
     },
+    // Query params for OIDC login, added by the Identity Provider, used as fallback when fragment is unsupported
+    oidc_query: {
+        keys: ["code", "state"],
+        location: "query",
+    },
     // 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"],

apps/web/test/unit-tests/components/structures/MatrixChat-test.tsx

@@ -321,7 +321,7 @@ describe("<MatrixChat />", () => {
         const code = "test-oidc-auth-code";
         const state = "test-oidc-state";
         const urlParams = {
-            oidc: {
+            oidc_fragment: {
                 code,
                 state: state,
             },
@@ -386,7 +386,7 @@ describe("<MatrixChat />", () => {
 
         it("should fail when query params do not include valid code and state", async () => {
             const urlParams = {
-                oidc: {
+                oidc_query: {
                     code: "",
                     state: "abc",
                 },

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("fragment");
+            expect(authUrl.searchParams.get("response_mode")).toEqual("query");
             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");
@@ -90,6 +90,18 @@ describe("OIDC authorization", () => {
             expect(authUrl.searchParams.has("nonce")).toBeTruthy();
             expect(authUrl.searchParams.has("code_challenge")).toBeTruthy();
         });
+
+        it("should prefer response_mode fragment if supported", async () => {
+            await startOidcLogin(
+                { ...delegatedAuthConfig, response_modes_supported: ["query", "fragment"] },
+                clientId,
+                homeserverUrl,
+            );
+
+            const authUrl = new URL(window.location.href);
+
+            expect(authUrl.searchParams.get("response_mode")).toEqual("fragment");
+        });
     });
 
     describe("completeOidcLogin()", () => {
@@ -131,19 +143,19 @@ describe("OIDC authorization", () => {
         });
 
         it("should throw when query params do not include state and code", async () => {
-            await expect(async () => await completeOidcLogin({})).rejects.toThrow(
+            await expect(async () => await completeOidcLogin({}, "query")).rejects.toThrow(
                 OidcClientError.InvalidQueryParameters,
             );
         });
 
         it("should make request complete authorization code grant", async () => {
-            await completeOidcLogin(params);
+            await completeOidcLogin(params, "fragment");
 
             expect(completeAuthorizationCodeGrant).toHaveBeenCalledWith(code, state, "fragment");
         });
 
         it("should return accessToken, configured homeserver and identityServer", async () => {
-            const result = await completeOidcLogin(params);
+            const result = await completeOidcLogin(params, "query");
 
             expect(result).toEqual({
                 accessToken: tokenResponse.access_token,

apps/web/test/unit-tests/vector/app-test.ts

@@ -76,7 +76,7 @@ describe("sso_redirect_options", () => {
         });
 
         it("should redirect for native OIDC", async () => {
-            const authConfig = makeDelegatedAuthConfig(issuer);
+            const authConfig = { ...makeDelegatedAuthConfig(issuer), response_modes_supported: ["query", "fragment"] };
             fetchMock.get("https://synapse/_matrix/client/v1/auth_metadata", authConfig);
             fetchMock.get(`${authConfig.issuer}.well-known/openid-configuration`, authConfig);
             fetchMock.get(authConfig.jwks_uri!, { keys: [] });

apps/web/test/unit-tests/vector/url_utils-test.ts

@@ -43,11 +43,18 @@ describe("parseUrlParameters", () => {
         expect(parsed.params.legacy_sso?.loginToken).toEqual("foobar");
     });
 
-    it("should parse oidc parameters from oauth-fragment", () => {
+    it("should parse oidc parameters from 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");
+        expect(parsed.params.oidc_fragment?.code).toEqual("foobar");
+        expect(parsed.params.oidc_fragment?.state).toEqual("barfoo");
+    });
+
+    it("should parse oidc parameters from query", () => {
+        const u = new URL("https://app.element.io/?code=foobar&state=barfoo");
+        const parsed = parseAppUrl(u);
+        expect(parsed.params.oidc_query?.code).toEqual("foobar");
+        expect(parsed.params.oidc_query?.state).toEqual("barfoo");
     });
 
     it("should parse guest parameters", () => {