supertokens / supertokens-website

Frontend SDK for SuperTokens - for session management + automatically refreshing sessions
https://supertokens.com
Other
54 stars 13 forks source link

localStorage Safari Security Error when users have "Block Cookies" setting enabled #232

Open zachequi opened 1 year ago

zachequi commented 1 year ago

Hi,

Safari does this funny thing where if you try and use localStorage when the user has enabled the "Block Cookies" setting the browser throws a Security Error, which bubbles up and can crash the parent application. We've seen 4 or 5 of these in production in the past 2 weeks, so its rare but does happen.

Some relevant links with background on this:

https://stackoverflow.com/questions/46632093/getting-security-error-on-iphone-when-using-localstorage

https://michalzalecki.com/why-using-localStorage-directly-is-a-bad-idea/#:~:text=SecurityError%3A%20The%20operation%20is%20insecure,localStorage%20object%20causes%20the%20error

https://hackernoon.com/why-localstorage-still-crashes-your-website-in-2023


I've also seen errors related to localStorage in browser-tabs-lock from Chrome Mobile Webviews. Looks like localStorage is null somehow on that platform.

image

nkshah2 commented 1 year ago

Hi @zachequi

We will look into the Safari issue, thanks. About the issue in chrome webviews, is this a webview launched inside of your mobile app?

nkshah2 commented 1 year ago

Hi @zachequi

We looked into the issue with Safari and the best way to get around this would be to provide a windowHandler when initialising SuperTokens.

windowHandler: (original) => {
        return {
            ...original,
            localStorage: {
                clear: () => {},
                clearSync: () => {},
                // TODO: add remaining functions
            },
        };
    },

For example if you are using store2 you could provide your own implementations for all the localStorage functions to use that instead of using the browser's localStorage.

You can find the full type of the object that the window handler expects here: https://github.com/supertokens/supertokens-website/blob/master/lib/ts/utils/windowHandler/types.ts.

Also about the issue in Android mobile webviews where localstorage is null, you may need to specifically enable storage in the webview when you launch it to make it available on Android. You can check here for how to do this: https://stackoverflow.com/questions/5899087/android-webview-localstorage/5934650#5934650

Closing this issue for now but feel free to re-open if you need more help

zachequi commented 1 year ago

Hi Nemi,

Maybe this is a difference of opinion but it seems weird to me that a library would put the burden of solving the safari edge case on calling applications rather than be robust and handle the scenario itself. How many other supertokens implementers will have users experience an application crash because they didn't know to add this override? That's certainly not good for super tokens, right?

On Thu, Sep 14, 2023, 3:44 AM Nemi Shah @.***> wrote:

Closed #232 https://github.com/supertokens/supertokens-website/issues/232 as completed.

— Reply to this email directly, view it on GitHub https://github.com/supertokens/supertokens-website/issues/232#event-10370652305, or unsubscribe https://github.com/notifications/unsubscribe-auth/A2KMBFY7STSBCQN3C6KIXJ3X2LN2NANCNFSM6AAAAAA4Q2MMVQ . You are receiving this because you were mentioned.Message ID: @.*** com>

nkshah2 commented 1 year ago

Hey @zachequi

While we agree this is not the best solution to the problem and we have added this to our pipeline we just cant promise when we will pick this up as a priority because so far this is the first complaint we have had about this. If we get more complaints about this we will move this up in terms of priority, in the meantime leaving this open so others can also see it. The suggestion for overriding it was just so that you can move past this issue in the meantime.

zachequi commented 1 year ago

Posting for the benefit of anyone who may find this down the road --- we've implemented store2 as the window handler as below. This mostly works, however we believe that there is an interaction between browser-tabs-lock and store2 that occasionally causes the whole tab to lock up. Still working through a solution to that.

 windowHandler: (original) => {
      return {
        ...original,
        // Override localstorage usage within supertokens to use store2 so we don't have
        // localStorage edgecase crashes.  https://github.com/supertokens/supertokens-website/issues/232#issuecomment-1719213092
        localStorage: {
          clear: () => (store2.clear() ? Promise.resolve() : Promise.resolve()),
          clearSync: store2.clear.bind(store2),

          key: (index) => Promise.resolve(store2.keys()[index]),
          keySync: (index) => store2.keys()[index],

          setItem: (key, value) => Promise.resolve(store2.set(key, value)),
          setItemSync: store2.set.bind(store2),

          getItem: (key) => Promise.resolve(store2.get(key)),
          getItemSync: store2.get.bind(store2),

          removeItem: (key) => Promise.resolve(store2.remove(key)),
          removeItemSync: store2.remove.bind(store2),
        },
      };