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
This commit is contained in:
Michael Telatynski 2026-04-16 12:07:39 +01:00 committed by GitHub
parent 583eae63f7
commit 64d3802efe
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
10 changed files with 75 additions and 30 deletions

View File

@ -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

View File

@ -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> {

View File

@ -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,

View File

@ -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:

View File

@ -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.

View File

@ -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"],

View File

@ -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",
},

View File

@ -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,

View File

@ -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: [] });

View File

@ -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", () => {