8000 Prevent Unexpected RESTART_AUTHENTICATION_ERROR on iOS browsers by t-tani · Pull Request #34709 · keycloak/keycloak · GitHub
[go: up one dir, main page]
Skip to content

Prevent Unexpected RESTART_AUTHENTICATION_ERROR on iOS browsers - #34709

Closed
t-tani wants to merge 1 commit into
keycloak:mainfrom
t-tani:issue/gh-33071-RESTART_AUTHENTICATION_ERROR-in-iphone-devices
Closed

Prevent Unexpected RESTART_AUTHENTICATION_ERROR on iOS browsers#34709
t-tani wants to merge 1 commit into
keycloak:mainfrom
t-tani:issue/gh-33071-RESTART_AUTHENTICATION_ERROR-in-iphone-devices

Conversation

@t-tani
@t-tani t-tani commented Nov 6, 2024 •
edited by ahus1
Loading
Copy link
Copy Markdown

Issue

Closes #33071

Unexpected requests to /realms/{realm_name}/login-actions/restart are causing RESTART_AUTHENTICATION_ERROR. This error overrides the OIDC client state (particularly affecting Cloudflare Zero Trust service) and can trigger CODE_TO_TOKEN_ERROR, resulting in unexpected unauthenticated errors.

Note: The occurrence of CODE_TO_TOKEN_ERROR is inconsistent and depends on the timing of the request to /realms/{realm_name}/login-actions/restart.

Root Cause

The checkCookiesAndSetTimer function in themes/src/main/resources/theme/base/login/resources/js/authChecker.js is not properly unloaded or cleared in the browser, leading to unexpected execution. This issue has been reproduced on iOS Safari and Google Chrome.

Comparison of network logs between environments where the problem does not occur (PC) and where it does (mobile) reveals an unexpected /realms/teamv/login-actions/restart request being sent.

Tested Environment

Test Cases

The following test cases were conducted using Passkey authentication on the default theme UI, with requests analyzed through a local proxy.

Test Case 1: Success (No error)

Environment:

  • Ubuntu 22.04
  • Google Chrome: Version 130.0.6723.91 (Official Build) (64-bit)

Normal authentication flow:

  1. GET /realms/{realm_name}/protocol/openid-connect/auth (200 OK)
    • loads checkCookiesAndSetTimer
    • set KC_RESTART cookie
  2. POST /realms/{realm_name}/login-actions/authenticate (input use 8000 rname) (200 OK)
    • loads checkCookiesAndSetTimer
  3. POST /realms/{realm_name}/login-actions/authenticate (input credentials) (302 Found)
    • unset KC_RESTART cookie
  4. GET /cdn-cgi/access/callback (302 Found)
  5. GET /cdn-cgi/access/authorized (302 Found)

Test Case 2: Failure (RESTART_AUTHENTICATION_ERROR)

Environment:

  • iOS 18.0.1
  • Safari 17.4 (19618.1.15.11.12)

Flow with RESTART_AUTHENTICATION_ERROR:

  1. GET /realms/{realm_name}/protocol/openid-connect/auth (200 OK)
    • loads checkCookiesAndSetTimer
    • set KC_RESTART cookie
  2. POST /realms/{realm_name}/login-actions/authenticate (input username) (200 OK)
    • loads checkCookiesAndSetTimer
  3. POST /realms/{realm_name}/login-actions/authenticate (input credentials) (302 Found)
    • unset KC_RESTART cookie
  4. GET /cdn-cgi/access/callback (302 Found)
  5. GET /cdn-cgi/access/authorized (302 Found)
  6. GET /realms/{realm_name}/login-actions/restart -> RESTART_AUTHENTICATION_ERROR

Web inspector analysis suggests that the timer unload code is not executed at the expected timing on iOS browsers:

addEventListener("beforeunload", () => {
  if (timeout) {
    clearTimeout(timeout);
    timeout = undefined;
  }
});

It appears the root cause was the beforeunload event not functioning correctly in iOS browsers. While the documentation at https://developer.mozilla.org/en-US/docs/Web/API/Window/beforeunload_event states that it's supported, in practice, it wasn't working as expected.

As a result, the restart request is invoked by the checkCookiesAndSetTimer function, even though the KC_RESTART cookie has already been cleared by valid authentication. This function/timer event is expected to be cleared after a valid authentication but does not in the case of access from iOS browsers.

Proposed Solution

Add validation before redirect to prevent unintended restart requests, as there should be no case where login-actions/restart is called without the KC_RESTART cookie being set.

Post-Fix Testing

Confirmed that the RESTART_AUTHENTICATION_ERROR no longer reproduces on iOS browsers after the fix.

Relevant Code for login-actions/restart

@t-tani
t-tani requested review from a team as code owners November 6, 2024 16:25
@ahus1
ahus1 requested a review from jonkoops November 7, 2024 09:39
@ahus1
ahus1 commented Nov 7, 2024
Copy link
Copy Markdown
Member

Pinging @jonkoops as you have triaged the original issue and asked for community help.

@jonkoops
jonkoops commented Nov 7, 2024
Copy link
Copy Markdown
Contributor

Yes, I'll make some time to get this reviewed. Thanks for the detailed information @t-tani, that helps us a lot.

@jonkoops
jonkoops commented Nov 7, 2024
Copy link
Copy Markdown
Contributor

Looks like some tests failed, so I did a quick rebase to make sure we're not just flaking out here. Let's see what the CI does.

@jonkoops
jonkoops commented Nov 7, 2024
Copy link
Copy Markdown
Contributor

Looks like the failures are consistent, @t-tani could you take a look and see why they are failing?

@t-tani
add cookie check before loginRestartUrl redirect
73f9ec8 
Signed-off-by: t-tani <31282248+t-tani@users.noreply.github.com>
@t-tani
t-tani commented Nov 8, 2024
Copy link
Copy Markdown
Author

@jonkoops

I apologize, but after investigating the test results, I discovered that the proposed fix was not effective. The KC_RESTART cookie has the http-only attribute, which means it cannot be accessed via JavaScript. As a result, the validation was not working as intended.

With the current implementation, I couldn't think of any solution other than addressing the issue of beforeunload not functioning on iOS browsers.

@t-tani
t-tani commented Nov 8, 2024
Copy link
Copy Markdown
Author

I will close this pull request. If we come up with any good ideas to fix the issue, I'll reopen this pull request.

@t-tani t-tani closed this Nov 8, 2024
@jonkoops
jonkoops commented Nov 8, 2024
Copy link
Copy Markdown
Contributor

If the KC_RESTART cookie is used as an internal cookie that does not contain any sensitive data I think we can expose it to the JavaScript side.

@t-tani
t-tani commented Nov 10, 2024
Copy link
Copy Markdown
Author

After testing, I've determined that using KC_RESTART does not resolve this issue.

Test procedure:

  1. Removed http-only attribute from KC_RESTART
  1. Added KC_RESTART validation before the redirect in checkCookiesAndSetTimer
  2. Ran CI tests

Failed tests:

  • multipleTabsParallelLoginTestWithAuthSessionExpiredAndResetPasswordClick
  • multipleTabsParallelLoginTestWithAuthSessionExpiredAndRequiredAction
  • multipleTabsParallelLoginTestWithAut 8000 hSessionExpiredInTheMiddle
  • multipleTabsParallelLoginTestWithAuthSessionExpiredAndRefreshInTab1
  • multipleTabsParallelLoginTestWithAuthSessionExpiredAndRegisterClick

Root cause:
The checkCookiesAndSetTimer function, triggered by setTimeout events in each tab, became dependent on KC_RESTART. This dependency interfered with expected redirects when KC_RESTART cookies were updated in other tabs.

Conclusion:
The KC_RESTART approach is not viable for solving this issue due to its impact on multi-tab auto redirect functionality.

@t-tani t-tani mentioned this pull request Nov 10, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jonkoops jonkoops Awaiting requested review from jonkoops

Assignees

No one assigned

Labels

team/core-clients

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

RESTART_AUTHENTICATION_ERROR in Iphone devices (using safari and chrome browser)

3 participants

@t-tani @ahus1 @jonkoops
0