Bug: BrowserSessionManager doesn't survive page reload

liborjelinek commented 7 months ago

Prerequisites

[X] I have searched the repository’s issues and Kinde community to ensure my issue isn’t a duplicate
[X] I have checked the latest version of the library to replicate my issue
[X] I have read the contributing guidelines
[X] I agree to the terms within the code of conduct

Describe the issue

I believe that shipped BrowserSessionManager is broken since it does not survive page reload. After hitting F5, Kinde client lost its authenticated state (kinde.isAuthenticated() telling false).

The setSessionItem()/setSessionItemBrowser(), getSessionItem()/getSessionItemBrowser(), and removeSessionItem()/removeSessionItem() pairs must to the same - persist a key to the sessionStorage, but the current implementation of BrowserSessionManager' non-Browser() methods save item to memory only.

Also, BrowserSessionManager destroySession() erases all items from sessionStorage (sessionStorage.clear()), not only its keys. Correct implementation must keep track of its keys.

I offer my sample implementation:

class SessionStorageBrowserSessionManager implements SessionManager {
  // Need to track all keys to delete on logout
  private PREFIX = "kinde_"

  private withPrefix(key: string): string {
    return this.PREFIX + key
  }

  async getSessionItem(itemKey: string) {
    return this.getSessionItemBrowser(itemKey)
  }
  async getSessionItemBrowser(itemKey: string) {
    itemKey = this.withPrefix(itemKey)
    return JSON.parse(sessionStorage.getItem(itemKey))
  }

  async setSessionItem(itemKey: string, itemValue: unknown) {
    this.setSessionItemBrowser(itemKey, itemValue)
  }
  async setSessionItemBrowser(itemKey: string, itemValue: unknown) {
    itemKey = this.withPrefix(itemKey)
    sessionStorage.setItem(itemKey, JSON.stringify(itemValue))
  }

  async removeSessionItem(itemKey: string) {
    this.removeSessionItemBrowser(itemKey)
  }
  async removeSessionItemBrowser(itemKey: string) {
    itemKey = this.withPrefix(itemKey)
    sessionStorage.removeItem(itemKey)
  }

  async destroySession() {
    // Called on logout
    // Collect keys with the prefix
    const keys = []
    for (let i = 0; i < sessionStorage.length; i++) {
        const key: string = sessionStorage.key(i)
        if (key.startsWith(this.PREFIX)) {
            keys.push(key)
        }
    }
    // Delete it
    for (let i = 0; i < keys.length; i++) {
        sessionStorage.removeItem(keys[i])
    }
    // Cannot do it within for loop because removeItem() change sessionStorage.length
  }
}

And later

export const kinde = createKindeBrowserClient({
  ...
  sessionManager: new SessionStorageBrowserSessionManager()
})

Library URL

https://github.com/kinde-oss/kinde-typescript-sdk

Library version

2.7.2

Operating system(s)

macOS

Operating system version(s)

macOS Sonoma

Further environment details

No response

Reproducible test case URL

No response

Additional information

No response

atifcppprogrammer commented 7 months ago

To anyone reading this please, view corresponding thread in slack community for more context.

DaveOrDead commented 7 months ago

I think it is important to denote the difference between development and production.

In production, the recommendation is that you use a custom domain (free on Kinde). This is because when a custom domain is present we set a first-party, secure, same-site, httpOnly cookie containing the refresh token which cannot be read by JS but can be posted to Kinde securely to persist the session and survive page reloads / new tabs etc.

For local development, you can use local or session storage to persist across page reloads by implementing a session manager instance.

Most build tools have a way of identitfying production vs development mode, for example in Vite you have the import.meta.env.MODE variable which resolves to production or development

Using Vite as an example, you could initiate the Kinde client with something like:

const kindeClient = createKindeBrowserClient({
  authDomain: <your_kinde_domain>,
  clientId: <your_kinde_client_id>,
  logoutRedirectURL: window.location,
  redirectURL: window.location,
  sessionManager: import.meta.env.MODE === "development" ? localSessionManager : null,
});

The key line really being sessionManager: import.meta.env.MODE === "development" ? localSessionManager : null, this will allow you to use a localSessionManager - (for example the sample in the original issue raised here) for local development and leverage the secure custom domain approach for production

liborjelinek commented 7 months ago

Thank you for clarification but it doesn't helping me :-(

I have prepared a minimal reproduction at https://github.com/liborjelinek/kinde-ts-sdk-page-reload-demo.

Checkout, npm init, npm run dev and go to https://localhost:5173 to see infinite redirect loop.

Entry point (/) is guarded by the function based on kinde.isAuthenticated().
On the very first opening, the it correctly says "not authenticated".
The guard function performs redirect to Kinde for login (location.href = await kinde.login()).
User sign-in.
Kinde calls registered callback page callback.html.
Callback page setup the auth state with kinde.handleRedirectToApp(new URL(window.location.toString())) and redirects back to /
The guard from / will wrongly say kinde.isAuthenticated() == false and thus redirect to Kinde for login.
An infinite loop happens here.

The prime suspect is builtin BrowserSessionManager. I made custom session manager.

After switching to it

export const kinde = createKindeBrowserClient({
   ...
  sessionManager: new SessionStorageBrowserSessionManager()
})

the kinde.isAuthenticated() is correctly saying true after return from Kinde.

coel commented 7 months ago

Hi @liborjelinek,

Thank you for taking the time to put that together. I've made a couple tweaks to get it to work: https://github.com/coel/kinde-ts-sdk-page-reload-demo

Note that this will only work when using a custom domain and your app is also running on the same domain (different sub-domain).

To explain a bit more. When using a custom domain we set a cookie containing the refresh token from our servers with HttpOnly, Secure and SameSite=Strict set so it can only be read by our servers. After page refresh, a request can be made to gain a new access token and this cookie will be used. This means the tokens do not need to be stored in local or session storage.

This is our recommended way to run production applications as it reduces the risk of tokens getting stolen by XSS. You can use local/session storage if that suits your app's security/risk profile, however we recommend only doing so for local development.

I can see the development experience can be improved though. Our original JS library (https://github.com/kinde-oss/kinde-auth-pkce-js/), for example, automatically attempts to get tokens on initialization, detects local development environment and has a is_dangerously_use_local_storage switch. We'll look at integrating these into this SDK.

If you have any further questions, just let us know.

liborjelinek commented 6 months ago

I see you have added calling refreshTokens(). This method of Kinde client is undocumented :-) and new to me. What does it do?

ama-michelange commented 6 months ago

Hi @liborjelinek,

Thank you for taking the time to put that together. I've made a couple tweaks to get it to work: https://github.com/coel/kinde-ts-sdk-page-reload-demo

Note that this will only work when using a custom domain and your app is also running on the same domain (different sub-domain).

To explain a bit more. When using a custom domain we set a cookie containing the refresh token from our servers with HttpOnly, Secure and SameSite=Strict set so it can only be read by our servers. After page refresh, a request can be made to gain a new access token and this cookie will be used. This means the tokens do not need to be stored in local or session storage.

This is our recommended way to run production applications as it reduces the risk of tokens getting stolen by XSS. You can use local/session storage if that suits your app's security/risk profile, however we recommend only doing so for local development.

I can see the development experience can be improved though. Our original JS library (https://github.com/kinde-oss/kinde-auth-pkce-js/), for example, automatically attempts to get tokens on initialization, detects local development environment and has a is_dangerously_use_local_storage switch. We'll look at integrating these into this SDK.

If you have any further questions, just let us know.

I think the Typescript SDK should behave in the same way as the Javascript SDK. And of course, browser Typescript SDK should automatically attempts to get tokens on initialization.

kinde-oss / kinde-typescript-sdk