Skip to content

Parse callback URL reliable #86

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
twobiers wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Parse callback URL reliable #86

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
1584476
Parse callback URL reliable
twobiers Jul 8, 2025
8e4741f
Use const and ternary for oauthState
twobiers Jul 9, 2025
5507e9b
Remove ternary alltogether as callback storage returns null
twobiers Jul 9, 2025
c34d6e8
Add ternary back to inline proper initialization
twobiers Jul 9, 2025
561c9cf
Clear params from callback URL only if location is redirect
twobiers Jul 9, 2025
7b7f284
Add test suite for paramter cleanup
twobiers Jul 11, 2025
0eac71e
Replace Location if callback was valid or on redirect location
twobiers Jul 11, 2025
931690c
Copy URL instead of mutating it
twobiers Jul 11, 2025
d0df222
Check if newUrl is present on the callback
twobiers Jul 11, 2025
f07c58a
Fix logic for getting redirect URI
twobiers Jul 11, 2025
859498a
Reorder steps in test to get a proper redirection simulated
twobiers Jul 11, 2025
1e8354e
Check if the redirect URI comes from a validated state
twobiers Jul 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 18 additions & 17 deletions lib/keycloak.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,7 @@ export default class Keycloak {
this.timeSkew = initOptions.timeSkew;
}

if(initOptions.redirectUri) {
if (initOptions.redirectUri) {
this.redirectUri = initOptions.redirectUri;
}

Expand Down Expand Up @@ -803,12 +803,20 @@ export default class Keycloak {
*/
async #processInit(initOptions) {
const callback = this.#parseCallback(window.location.href);
const redirectFromState = callback?.valid ? callback.redirectUri : null;
const redirectUri = redirectFromState || initOptions.redirectUri || this.redirectUri;
const callbackValid = callback && callback.valid;
const onRedirectLocation = redirectUri && window.location.href.startsWith(redirectUri);

if (callback?.redirectUri) {
window.history.replaceState(window.history.state, '', callback.redirectUri);
// 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?.redirectUri && (callbackValid || onRedirectLocation)) {
window.history.replaceState(window.history.state, '', /** @type {string} */(callback.redirectUri));
}

if (callback && callback.valid) {
if (callbackValid) {
await this.#setupCheckLoginIframe();
await this.#processCallback(callback);
return;
Expand Down Expand Up @@ -1017,7 +1025,7 @@ export default class Keycloak {
return;
}

var oauthState = this.#callbackStorage.get(oauth.state);
const oauthState = oauth.state ? this.#callbackStorage.get(oauth.state) : null;

if (oauthState) {
oauth.valid = true;
Expand Down Expand Up @@ -1066,19 +1074,12 @@ export default class Keycloak {
redirectUri = url.toString();
}

if (parsed?.oauthParams) {
if (this.flow === 'standard' || this.flow === 'hybrid') {
if ((parsed.oauthParams.code || parsed.oauthParams.error) && parsed.oauthParams.state) {
Copy link
Contributor

@jonkoops jonkoops Jul 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not quite sure why we had this check here before, perhaps for security reasons, or to prevent accidental redirects. However, I agree, there is no reason to keep this code around, we validate the callbacks later on either way.

Copy link
Contributor

@jonkoops jonkoops Jul 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upon closer inspection, this is almost certainly to prevent accidentally stripping some parameters by accident, see #86 (review)

parsed.oauthParams.redirectUri = redirectUri;
return parsed.oauthParams;
}
} else if (this.flow === 'implicit') {
if ((parsed.oauthParams.access_token || parsed.oauthParams.error) && parsed.oauthParams.state) {
parsed.oauthParams.redirectUri = redirectUri;
return parsed.oauthParams;
}
}
if (!(parsed && parsed.oauthParams)) {
return;
}

parsed.oauthParams.redirectUri = redirectUri;
return parsed.oauthParams;
}

/**
Expand Down
100 changes: 99 additions & 1 deletion test/tests/init.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,106 @@
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 })
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<URL>, 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(() => {
return new Promise((resolve) => setTimeout(resolve, 0));
Comment on lines +61 to +62
Copy link

Copilot AI Jul 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Using a zero-delay setTimeout for waiting is brittle; consider using page.waitForURL() or listening for a history event to get a more deterministic synchronization point.

Suggested change
await page.evaluate(() => {
return new Promise((resolve) => setTimeout(resolve, 0));
await page.waitForURL((url) => {
const cleanedUrl = new URL(url);
return !params.some(param => responseMode === 'query' ? cleanedUrl.searchParams.has(param) : cleanedUrl.hash.includes(`${param}=`));

Copilot uses AI. Check for mistakes.
Copy link
Author

@twobiers twobiers Jul 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My idea here was to wait for all (async) JS to be executed before running assertions. I didn't take a closer look at executor.initializeAdapter yet. Maybe this call is not needed.

waitForUrl will slow down test execution in error cases

});

// 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(() => {
return 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}=`);
}
}
});
});