itpropro / nuxt-oidc-auth

OIDC (OpenID connect) focused auth module for Nuxt
https://nuxtoidc.cloud
MIT License
88 stars 23 forks source link

Logout without confirm screen fails due to missing id_token_hint #79

Open nettnikl opened 1 month ago

nettnikl commented 1 month ago

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 https://github.com/keycloak/keycloak/discussions/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).

https://github.com/itpropro/nuxt-oidc-auth/blob/1c41ec6ace19bef01c023b9a806774fcac420dc1/src/runtime/server/handler/logout.get.ts#L27

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 https://github.com/itpropro/nuxt-oidc-auth/blob/1c41ec6ace19bef01c023b9a806774fcac420dc1/src/runtime/server/utils/session.ts#L160 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 commented 1 month ago

Also, i'm not sure, but i suppose it could be connected to, or even the same as https://github.com/itpropro/nuxt-oidc-auth/issues/40

brucetony commented 1 month ago

This is the same issue as mentioned in https://github.com/itpropro/nuxt-oidc-auth/issues/62 and I believe his beta build fixes this already. Just waiting for the next release

nettnikl commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

In https://github.com/itpropro/nuxt-oidc-auth/commit/6eca9598bdbe66b1fefa565ee4a13ef5d4986312 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 commented 1 month ago

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