Skip to content
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.

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

Logout without confirm screen fails due to missing id_token_hint #79

Closed
nettnikl opened this issue Oct 27, 2024 · 7 comments · May be fixed by #80
Closed

Logout without confirm screen fails due to missing id_token_hint #79

nettnikl opened this issue Oct 27, 2024 · 7 comments · May be fixed by #80

Comments

@nettnikl
Copy link

Hi,

i analyzed an issue i have with the logout process using this library with a keycloak backend.

Issue

After a logout-click, the user is stuck on the keycloak-logout-failed-page due to a "Invalid parameter: id_token_hint"

Workaround

Manually changing the url from
http://localhost:8080/realms/master/protocol/openid-connect/logout?post_logout_redirect_uri=http:%2F%2Flocalhost:3000&id_token_hint
to
localhost:8080/realms/master/protocol/openid-connect/logout?post_logout_redirect_uri=http:%2F%2Flocalhost:3000&client_id=my-oauth2-app
shows the keycloak-confirm-logout-page, working correctly. But this enforces the user to confirm the logout.

This behavior is expected, as i found this is described here as well
keycloak/keycloak#12183

Root cause

Seeing that the template for keyvault provided by this lib sets exposeIdToken by default, it seems clear to me the intended way is to not require the client_id logout flow.
(which results in the following check magically setting the url parameter to the idToken (which is stored in the session when exposeIdToken is true).

additionalLogoutParameters[key] = userSession.idToken

Debugging reveals that idToken is not set in the userSession, so the value is not magically set, but stays set (not null), but empty. I suppose it is meant to be there, as the template explicitly sets exposeIdToken, which "exposes the raw id token to the client within session object".

This results in the error page.

Potential fix

I suspect the bug in the session.js:
In the function getUserSession > if(providerSessionConfigs[provider]?.expirationCheck) > if(expired) > if (providerSessionConfigs[provider].automaticRefresh), we return without the if (useRuntimeConfig(event).oidc.providers[provider]?.exposeAccessToken || providerPresets[provider].exposeAccessToken) check that happens a few lines after.

Therefore, the value is retrieved (i checked - it definitely is there during the execution of refreshUserSession() happening one line above the return), but not stored in the session, despite the enabled setting.

I propose to add this

      logger.info("Session expired");
      if (providerSessionConfigs[provider].automaticRefresh) {
        await refreshUserSession(event);
        // XXXX
        if (useRuntimeConfig(event).oidc.providers[provider]?.exposeIdToken || providerPresets[provider].exposeIdToken) {
          const persistentSession = await useStorage("oidc").getItem(session.id);
          const tokenKey = process.env.NUXT_OIDC_TOKEN_KEY;
          if (persistentSession?.idToken)
            userSession.idToken = await decryptToken(persistentSession.idToken, tokenKey) || void 0;
        }
        // XXXX
        return userSession;
      }

I changed it locally, resolving the bug.

Required help

The MR is not an issue, i'll add the lines to

await refreshUserSession(event)

But i need a review, because i', not sure if i understood the specifications of OIDC correctly, or if this change has further unseen security implications.

nettnikl added a commit to nettnikl/nuxt-oidc-auth that referenced this issue Oct 27, 2024
Fixes itpropro#79, logout without confirm screen fails due to missing id_token_hint
@nettnikl
Copy link
Author

Also, i'm not sure, but i suppose it could be connected to, or even the same as #40

@brucetony
Copy link

This is the same issue as mentioned in #62 and I believe his beta build fixes this already. Just waiting for the next release

@nettnikl
Copy link
Author

Yeah i've seen this issue, already upvoted. Just - i'm on the beta build 1.0.0-beta.2, so i would assume it doesnt fix my issue. I'll check the diff.

@nettnikl
Copy link
Author

Can't really spot the diff that would fix my issue, could you kindly link to it @brucetony ? Im currently on mobile, so i might have overlooked it.

@brucetony
Copy link

brucetony commented Oct 28, 2024

In 6eca959 he updated the logout method to clear the session and redirect the user. I've tested his 1.0.0-beta.1 build and it does indeed fix the id_token_hint bug

@brucetony
Copy link

Upon further testing, the new beta builds do not resolve the bug so your proposed solution does appear to be the best solution going forward

@itpropro
Copy link
Owner

itpropro commented Jan 6, 2025

The id_token_hint problems should all be fixed since 1.0.0-beta.1 and 1.0.0-beta.2.
I was not able to reproduce any of the issues with keycloak with the current version.
Adding the id_token to an expired session would be considered as a serious security issue and can also lead to unexpected side effects.
I would close this issue for now, feel free to reopen it, if you have any further problems with the id_token_hint of keycloak.

@itpropro itpropro closed this as completed Jan 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants