Parse callback URL reliable #86

twobiers · 2025-04-29T08:59:01Z

This PR partially addresses #85 by parsing the redirect params more reliable and remove them from the window location.

With this change in place the state parameter is not required to properly parse the redirect params and remove them from the window location.
The state is still required for a successful authorization flow, so if the state is not present, keycloak-js will remove the parameters but doesn't treat it as valid.

That would fix the issue in our particular context where the WAF is blocking the second login attempt due to double encoding

jonkoops · 2025-07-08T13:20:29Z

Thanks for the PR @twobiers, could I ask you to do a rebase? I have made some significant changes, but the general logic should be intact.

twobiers · 2025-07-08T13:47:34Z

@jonkoops Of course you can ask^^
I had some troubles with the vscode merge editor, but the branch is now rebased.

jonkoops · 2025-07-09T10:21:12Z

Side note, this is a security sensitive area, as the state parameter is required to prevent a category of CSRF attacks (see #133). It looks like the changes here don't impact this, as mentioned by @twobiers in the description, but this is something we need to keep in mind during the review.

jonkoops · 2025-07-09T10:46:18Z

lib/keycloak.js


-        if (parsed && parsed.oauthParams) {
-            if (this.flow === 'standard' || this.flow === 'hybrid') {
-                if ((parsed.oauthParams.code || parsed.oauthParams.error) && parsed.oauthParams.state) {


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.

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

jonkoops

This PR generally looks good to me, I just want to make sure this does not introduce any unexpected behavior. I am worried the client application could be using some of the parameters in supportedParams as their own state. For example:

http://myapp.com/?error=someerror

In this case, will initializing Keycloak JS remove this parameter from the URL? That might break expectations of the client. This is likely why newUrl was guarded before on the required paramaters being present (e.g. code, access_token, error).

twobiers · 2025-07-09T11:35:56Z

@jonkoops That is a very valid concern I didn't have on my mind. Keycloak-js will only replace the location when parseCallbackUrl() is returning a value.
https://github.com/twobiers/keycloak-js/blob/90a00f37048f15015ec89184b36d47cc43918723/lib/keycloak.js#L805-L809

This will be the case when at least one oauth parameter from supportedParams can be parsed from the url:
https://github.com/twobiers/keycloak-js/blob/90a00f37048f15015ec89184b36d47cc43918723/lib/keycloak.js#L1078-L1080

I think we could guard against this by only replacing the window location if it is the desired redirectUri from the authentication request. What do you think?

jonkoops · 2025-07-10T08:29:11Z

I think we could guard against this by only replacing the window location if it is the desired redirectUri from the authentication request. What do you think?

Could you elaborate a little on how this would work? I am trying to imagine all the various scenarios, feel free to push a commit with some implementation if that is easier.

twobiers · 2025-07-10T10:19:28Z

See 1f632f5

This will only strip parameters if on a configured redirect uri. Either globally or from a previous login request.
If keycloak-js is initialized on some other location, parameters remain.

So in the case where an authorization flow triggered externally (like registering the user over API, passing redirect URI through E-Mail) the parameters will get removed if the redirect URI matches the one configured in keycloak-js globally.

This is the only valid solution I could come up with, without taking the risk of removing random unrelated parameters.

jonkoops · 2025-07-10T10:43:09Z

Just did a little bit of testing, and this bug already seems to exist, although this PR does intensify it. For example, if I use the following URL:

http://myapp.com/?code=blabla

If we now initialize Keycloak JS, it will remove the code parameter from the URL. Ideally, all parameters would be preserved as they were in the original URL.

jonkoops · 2025-07-10T10:45:35Z

@twobiers let's try your solution and do some testing. Can you push your commit?

twobiers · 2025-07-10T11:03:37Z

@jonkoops I have pushed everything I have. Are you missing something?
Just to point out, the new diff Preview in GitHub might behave differently, maybe you're using it?

I would like to bring some unit tests to this branch aswell. Not very familiar with the test setup and if it's testable nice, but I would like to give it a shot.

jonkoops · 2025-07-10T11:36:44Z

Just to point out, the new diff Preview in GitHub might behave differently, maybe you're using it?

I am, and I was looking at an old diff, so that explains. It should be pretty easy to add some tests to validate this behavior, check out the instructions in the test/ directory.

jonkoops · 2025-07-10T11:54:51Z

Looks like the changes as they are now will cause parameters not to be removed from the URL after signing in successfully. We'll have to come up with a different solution.

jonkoops · 2025-07-10T12:42:12Z

I am going to raise some PR(s) to clean up the existing logic around parsing callback URLs, I'll post here when I have something.

twobiers · 2025-07-11T07:39:33Z

Looks like the changes as they are now will cause parameters not to be removed from the URL after signing in successfully. We'll have to come up with a different solution.

I see. I've added a (probably too broad to keep) test suite to validate the desired outcome, and the functionality works like I expected it, but it looks like the change breaks other assumption.
While I'm fiddling around here, I convert this PR to draft state again.

twobiers · 2025-07-11T13:21:23Z

@jonkoops I think I found an option that is compatible with the previous behavior and removes parameters in a reasonable approach.

The condition to parameter removal is now either a valid callback from a previous login attempt OR the user agent is located on an explicitly configured redirect URI.

~~The login.spec.ts tests are flaky for me now, but I should be able to resolve that too~~ (fixed)

Copilot

Pull Request Overview

This PR improves the reliability of parsing and clearing OAuth callback parameters from window.location and adds end-to-end tests to verify that behavior across flows and response modes.

Refactor #processInit to only replace URL when on a known redirect URI or after a valid callback
Simplify callback parameter parsing logic and tighten state lookup
Add Playwright tests to confirm removal/preservation of parameters for standard, implicit, and hybrid flows

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
test/tests/init.spec.ts	Adds parameterized tests to verify removal or retention of auth response parameters
lib/keycloak.js	Refines URL replacement in `#processInit`, callback validation, and parsing logic

Comments suppressed due to low confidence (2)

test/tests/init.spec.ts:38

[nitpick] The helper name addRandomParams is ambiguous—consider renaming it to something like addAuthResponseParams to clarify that it appends OAuth callback parameters.

  const addRandomParams = (url: Readonly<URL>, params: string[], mode: string) => {

test/tests/init.spec.ts:37

Only fragment mode is tested for implicit and hybrid flows. If query-mode is supported for these flows, add test cases for [implicit / query] and [hybrid / query] to improve coverage.

].forEach(({ flow, responseMode, params }) => {

lib/keycloak.js

Copilot · 2025-07-11T13:59:40Z

test/tests/init.spec.ts

+    await page.evaluate(() => {
+      return new Promise((resolve) => setTimeout(resolve, 0));


[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}=`));

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

jonkoops · 2025-07-11T14:34:45Z

Thanks @twobiers, I'll try to squeeze in a review. I'd like to do some small refactoring first, you can see the progress of that under #143.

jonkoops · 2025-07-16T15:30:34Z

@twobiers could I ask you to do a rebase on main now that #143 has been merged?

Signed-off-by: twobiers <[email protected]>

Co-authored-by: Jon Koops <[email protected]> Signed-off-by: Tobi <[email protected]>

Signed-off-by: twobiers <[email protected]>

twobiers · 2025-07-16T16:28:48Z

@twobiers could I ask you to do a rebase on main now that #143 has been merged?

Rebased and tested again.
I'm not a 100% sure (it's late), but I think #143 now has introduced the behavior from your comment #86 (review) by removing newUrl and consolidating it with redirectUri.

Now redirectUri has two meanings.
Here (oauth.redirectUri) it is the URI to relocate to with oauth paramters removed.

keycloak-js/lib/keycloak.js

Line 1015 in 7a67a97

var oauth = this.#parseCallbackUrl(url);

And here it is the redirect URI used for the Authorization Request.

keycloak-js/lib/keycloak.js

Line 1024 in 7a67a97

oauth.redirectUri = oauthState.redirectUri;

There is an overlap between these two, and they share semantics, however for the sake of this PR it means I had to introduce an additional check 1e8354e (#86).

twobiers force-pushed the fix/clear-response-params branch 2 times, most recently from c232659 to 357d19f Compare July 8, 2025 13:44

jonkoops requested review from a team, Copilot and jonkoops July 9, 2025 10:21

This comment was marked as outdated.

Sign in to view

jonkoops reviewed Jul 9, 2025

View reviewed changes

twobiers marked this pull request as draft July 11, 2025 07:39

twobiers marked this pull request as ready for review July 11, 2025 13:09

twobiers requested review from Copilot and jonkoops July 11, 2025 13:58

Copilot AI reviewed Jul 11, 2025

View reviewed changes

twobiers force-pushed the fix/clear-response-params branch from d371a06 to 2422d7c Compare July 16, 2025 16:08

twobiers and others added 12 commits July 16, 2025 18:19

Parse callback URL reliable

1584476

Signed-off-by: twobiers <[email protected]>

Use const and ternary for oauthState

8e4741f

Co-authored-by: Jon Koops <[email protected]> Signed-off-by: Tobi <[email protected]>

Remove ternary alltogether as callback storage returns null

5507e9b

Signed-off-by: twobiers <[email protected]>

Add ternary back to inline proper initialization

c34d6e8

Signed-off-by: twobiers <[email protected]>

Clear params from callback URL only if location is redirect

561c9cf

Signed-off-by: twobiers <[email protected]>

Add test suite for paramter cleanup

7b7f284

Signed-off-by: twobiers <[email protected]>

Replace Location if callback was valid or on redirect location

0eac71e

Signed-off-by: twobiers <[email protected]>

Copy URL instead of mutating it

931690c

Signed-off-by: twobiers <[email protected]>

Check if newUrl is present on the callback

d0df222

Signed-off-by: twobiers <[email protected]>

Fix logic for getting redirect URI

f07c58a

Signed-off-by: twobiers <[email protected]>

Reorder steps in test to get a proper redirection simulated

859498a

Signed-off-by: twobiers <[email protected]>

Check if the redirect URI comes from a validated state

1e8354e

Signed-off-by: twobiers <[email protected]>

twobiers force-pushed the fix/clear-response-params branch from 2422d7c to 1e8354e Compare July 16, 2025 16:19

		await page.evaluate(() => {
		return new Promise((resolve) => setTimeout(resolve, 0));

-    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}=`));

Parse callback URL reliable #86

Are you sure you want to change the base?

Parse callback URL reliable #86

Uh oh!

Conversation

twobiers commented Apr 29, 2025

Uh oh!

jonkoops commented Jul 8, 2025

Uh oh!

twobiers commented Jul 8, 2025

Uh oh!

jonkoops commented Jul 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

jonkoops Jul 9, 2025

Choose a reason for hiding this comment

Uh oh!

jonkoops Jul 10, 2025

Choose a reason for hiding this comment

Uh oh!

jonkoops left a comment

Choose a reason for hiding this comment

Uh oh!

twobiers commented Jul 9, 2025

Uh oh!

jonkoops commented Jul 10, 2025

Uh oh!

twobiers commented Jul 10, 2025

Uh oh!

jonkoops commented Jul 10, 2025

Uh oh!

jonkoops commented Jul 10, 2025

Uh oh!

twobiers commented Jul 10, 2025

Uh oh!

jonkoops commented Jul 10, 2025

Uh oh!

jonkoops commented Jul 10, 2025

Uh oh!

jonkoops commented Jul 10, 2025

Uh oh!

twobiers commented Jul 11, 2025

Uh oh!

twobiers commented Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Copilot AI Jul 11, 2025

Choose a reason for hiding this comment

Uh oh!

twobiers Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonkoops commented Jul 11, 2025

Uh oh!

jonkoops commented Jul 16, 2025

Uh oh!

twobiers commented Jul 16, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jonkoops commented Jul 9, 2025 •

edited

Loading

twobiers commented Jul 11, 2025 •

edited

Loading

twobiers Jul 11, 2025 •

edited

Loading