diff --git a/lib/keycloak.js b/lib/keycloak.js index 7afa0be..5c90729 100755 --- a/lib/keycloak.js +++ b/lib/keycloak.js @@ -806,12 +806,20 @@ export default class Keycloak { */ async #processInit (initOptions) { const callback = this.#parseCallback(window.location.href) + const redirectFromState = callback?.valid ? callback.newUrl : null + const redirectUri = redirectFromState || initOptions.redirectUri || this.redirectUri + const callbackValid = callback && callback.valid + const onRedirectLocation = redirectUri && window.location.href.startsWith(redirectUri) - if (callback?.newUrl) { - window.history.replaceState(window.history.state, '', callback.newUrl) + // We relocate either if we detected that the callback was valid or if we're currently on a configured redirect URI. + // Only then, we can be sure that the URL is safe to replace, because an application could make use of + // http://myapp.com/?error=someerror to indicate an unrelated error and it would be cleared as part of the keycloak + // initialization process. + if (callback?.newUrl && (callbackValid || onRedirectLocation)) { + window.history.replaceState(window.history.state, '', /** @type {string} */(callback.newUrl)) } - if (callback && callback.valid) { + if (callbackValid) { await this.#setupCheckLoginIframe() await this.#processCallback(callback) return @@ -1019,7 +1027,7 @@ export default class Keycloak { return } - const oauthState = this.#callbackStorage.get(oauth.state) + const oauthState = oauth.state ? this.#callbackStorage.get(oauth.state) : null if (oauthState) { oauth.valid = true @@ -1068,19 +1076,12 @@ export default class Keycloak { newUrl = url.toString() } - if (parsed?.oauthParams) { - if (this.flow === 'standard' || this.flow === 'hybrid') { - if ((parsed.oauthParams.code || parsed.oauthParams.error) && parsed.oauthParams.state) { - parsed.oauthParams.newUrl = newUrl - return parsed.oauthParams - } - } else if (this.flow === 'implicit') { - if ((parsed.oauthParams.access_token || parsed.oauthParams.error) && parsed.oauthParams.state) { - parsed.oauthParams.newUrl = newUrl - return parsed.oauthParams - } - } + if (!(parsed && parsed.oauthParams)) { + return } + + parsed.oauthParams.newUrl = newUrl + return parsed.oauthParams } /** diff --git a/test/tests/init.spec.ts b/test/tests/init.spec.ts index aa61ae1..cd9713a 100644 --- a/test/tests/init.spec.ts +++ b/test/tests/init.spec.ts @@ -1,5 +1,6 @@ import { expect } from '@playwright/test' import { createTestBed, test } from '../support/testbed.ts' +import type { KeycloakFlow, KeycloakResponseMode } from '../../lib/keycloak.js' test('throws when initializing multiple times', async ({ page, appUrl, authServerUrl }) => { const { executor } = await createTestBed(page, { appUrl, authServerUrl }) @@ -7,3 +8,100 @@ test('throws when initializing multiple times', async ({ page, appUrl, authServe await executor.initializeAdapter() await expect(executor.initializeAdapter()).rejects.toThrow("A 'Keycloak' instance can only be initialized once.") }) + +const standardParams = ['code', 'state', 'session_state', 'kc_action_status', 'kc_action', 'iss'] +const implicitParams = ['access_token', 'token_type', 'id_token', 'state', 'session_state', 'expires_in', 'kc_action_status', 'kc_action', 'iss'] +const hybridParams = ['access_token', 'token_type', 'id_token', 'code', 'state', 'session_state', 'expires_in', 'kc_action_status', 'kc_action', 'iss'] +const errorParams = ['error', 'error_description', 'error_uri']; + +[ + { + flow: 'standard', + responseMode: 'fragment', + params: [...standardParams, ...errorParams] + }, + { + flow: 'standard', + responseMode: 'query', + params: [...standardParams, ...errorParams] + }, + { + flow: 'implicit', + responseMode: 'fragment', + params: [...implicitParams, ...errorParams] + }, + { + flow: 'hybrid', + responseMode: 'fragment', + params: [...hybridParams, ...errorParams] + } +].forEach(({ flow, responseMode, params }) => { + const addRandomParams = (url: Readonly, params: string[], mode: string) => { + const newUrl = new URL(url) + for (const param of params) { + if (mode === 'query') { + newUrl.searchParams.set(param, `test-${param}`) + } else { + newUrl.hash = `${newUrl.hash ? newUrl.hash + '&' : ''}${param}=test-${param}` + } + } + return newUrl + } + + test(`[${responseMode} / ${flow}] should remove authorization response parameters from redirect URL`, async ({ page, appUrl, authServerUrl }) => { + const { executor } = await createTestBed(page, { appUrl, authServerUrl }) + const redirect = addRandomParams(appUrl, params, responseMode) + + await page.goto(redirect.toString()) + await executor.initializeAdapter({ + responseMode: responseMode as KeycloakResponseMode, + flow: flow as KeycloakFlow, + redirectUri: appUrl.toString() + }) + // Wait for the adapter to process the redirect and clean up the URL + await page.evaluate(async () => { + return await new Promise((resolve) => setTimeout(resolve, 0)) + }) + + // Check that the URL has been cleaned up + const currentUrl = page.url() + const url = new URL(currentUrl) + for (const param of params) { + if (responseMode === 'query') { + expect(url.searchParams.has(param)).toBe(false) + } else { + expect(url.hash).not.toContain(`${param}=`) + } + } + }) + + test(`[${responseMode} / ${flow}] should preserve parameters from the URL on non-redirect pages`, async ({ page, appUrl, authServerUrl }) => { + const { executor } = await createTestBed(page, { appUrl, authServerUrl }) + + // Visit the App URL before initialization + const newAppUrl = addRandomParams(appUrl, params, responseMode) + await page.goto(newAppUrl.toString()) + + const redirectUri = new URL('callback', newAppUrl) + await executor.initializeAdapter({ + responseMode: responseMode as KeycloakResponseMode, + flow: flow as KeycloakFlow, + redirectUri: redirectUri.toString() + }) + // Wait for the adapter to process the redirect and possibly clean up the URL + await page.evaluate(async () => { + return await new Promise((resolve) => setTimeout(resolve, 0)) + }) + + // Check that the URL has NOT been cleaned up + const currentUrl = page.url() + const url = new URL(currentUrl) + for (const param of params) { + if (responseMode === 'query') { + expect(url.searchParams.has(param)).toBe(true) + } else { + expect(url.hash).toContain(`${param}=`) + } + } + }) +})