Bug report

Describe the bug

The reference to globalThis.localStorage results in an exception which breaks supabase due to specifics of the environment, which is internal browser page (in this case, about:preferences). There is no concept of localStorage. It isn't clear what the purpose is of localStorage, but the presence of persistSession and the code seems to indicate it is simply to store and recover the session.

If persistSession is set to false, I wouldn't expect localStorage to be required (either on globalThis, or an alternative provide), but that isn't the case due to the following line:

https://github.com/supabase/gotrue-js/blob/master/src/GoTrueClient.ts#L99

" this.localStorage = settings.localStorage || globalThis.localStorage"

This line requires one of them to be implemented. That would be fine except that attempting to access globalThis.localStorage generates an exception in my environment.

To Reproduce

Not exactly the same but you can see the exception that is causing the issue:

1. In Firefox open about:preferences
2. Open '...' -> More Tools -> Web Developer Tools
3. In the console, type globalThis.localStorage and hit enter
4. See error:

>> globalThis.localStorage
Uncaught 
Exception { name: "NS_ERROR_NOT_AVAILABLE", message: "", result: 2147746065, filename: "debugger eval code", lineNumber: 1, columnNumber: 0, data: null, stack: "@debugger eval code:1:1\ngetEvalResult@resource://devtools/server/actors/webconsole/eval-with-debugger.js:243:24\nexports.evalWithDebugger@resource://devtools/server/actors/webconsole/eval-with-debugger.js:167:14\nevaluateJS@resource://devtools/server/actors/webconsole.js:1118:38\nevaluateJSAsync/<@resource://devtools/server/actors/webconsole.js:1010:35\nexports.makeInfallible/<@resource://devtools/shared/ThreadSafeDevToolsUtils.js:103:22\n", location: XPCWrappedNative_NoHelper }

Expected behavior

I expect it to not access globalThis.localStorage if persistSession is false. Alternative, the exception should be caught.

System information

• OS: macos
• Browser (if applies) Firefox 104.0.2 (64-bit)
• Version of supabase-js: 1.35.6
• Version of Node.js: v16.15.0

The following diff should do the trick but I've not been able to test due to difficulties build:

diff --git a/src/GoTrueClient.ts b/src/GoTrueClient.ts
index 900bc55..021798c 100644
--- a/src/GoTrueClient.ts
+++ b/src/GoTrueClient.ts
@@ -61,11 +61,11 @@ export default class GoTrueClient {

   protected autoRefreshToken: boolean
   protected persistSession: boolean
-  protected localStorage: SupportedStorage
+  protected localStorage: SupportedStorage | null
   protected multiTab: boolean
   protected stateChangeEmitters: Map<string, Subscription> = new Map()
   protected refreshTokenTimer?: ReturnType<typeof setTimeout>
-  protected networkRetries: number = 0
+  protected networkRetries = 0

   /**
    * Create a new client for use in the browser.
@@ -96,7 +96,11 @@ export default class GoTrueClient {
     this.autoRefreshToken = settings.autoRefreshToken
     this.persistSession = settings.persistSession
     this.multiTab = settings.multiTab
-    this.localStorage = settings.localStorage || globalThis.localStorage
+    if (settings.persistSession) {
+      this.localStorage = settings.localStorage || globalThis.localStorage
+    } else {
+      this.localStorage = null
+    }
     this.api = new GoTrueApi({
       url: settings.url,
       headers: settings.headers,
diff --git a/src/lib/types.ts b/src/lib/types.ts
index fd2d7d2..c0d2829 100644
--- a/src/lib/types.ts
+++ b/src/lib/types.ts
@@ -221,4 +221,6 @@ type PromisifyMethods<T> = {
     : T[K]
 }

-export type SupportedStorage = PromisifyMethods<Pick<Storage, 'getItem' | 'setItem' | 'removeItem'>>
+export type SupportedStorage = PromisifyMethods<
+  Pick<Storage, 'getItem' | 'setItem' | 'removeItem'>
+> | null

FWIW, I tried to implement the diff above, but I gave up due to the multiple packages that needed building. I hope someone with a currently working dev environment could give it a quick test.

@davidmaxwaterman localstorage indeed is used to store and recover the session.

Unfortunately the required change to fix your issue in v1 isn't as easy as your above diff. There are plenty of calls on this.localStorage.(set|get|remove)Item() so setting this.localStorage to null would throw errors all over the place. As an example check _recoverSession() that gets called a couple lines below your intended changes. It passes localStorage to getItemSynchronously that then calls localStorage.getItem().

https://github.com/supabase/gotrue-js/blob/5609423858cb86cb8eebdaeee662e7598b4e20ab/src/GoTrueClient.ts#L630-L633

I created a draft PR #436, that tries to fix your issue in the release candidate of v2. Unfortunately i can't really test it as i don't have a browser without localStorage. Could you try it in your project? You would have to install supabase-js@rc, clone and checkout the PR and run npm install && npm run build. Then you could link it into your project.

For v1 the best workaround is probably to either pass an in memory storage or a noop storage. Both should be fine. An in memory storage is more likely to behave like a usual browser client.

class InMemoryStorage {
  constructor() {
    this._storage = new Map();
  }

  setItem(key, value) {
    this._storage.set(key, value);
  }  
 
  getItem(key) {
    return this._storage.has(key) ? this._storage.get(key) : null;
  }

  removeItem(key) {
    this._storage.delete(key);
  }  
}

const supabase = createClient(URL, ANON_KEY, {localStorage: new InMemoryStorage()})

class NoopStorage {
  setItem(key, value) {}  
 
  getItem(key) {
    return null;
  }

  removeItem(key) {}  
}

const supabase = createClient(URL, ANON_KEY, {localStorage: new NoopStorage()})

Unfortunately the required change to fix your issue in v1 isn't as easy as your above diff. There are plenty of calls on this.localStorage.(set|get|remove)Item() so setting this.localStorage to null would throw errors all over the place

Are you sure?

I guess I didn't check them all, but the ones I saw were all protected with optional chaining eg:

https://github.com/supabase/gotrue-js/blob/5609423858cb86cb8eebdaeee662e7598b4e20ab/src/lib/helpers.ts#L63

TBH, I found that odd since the TS definitions supposedly prevented it from being null/undefined.

For v1 the best workaround is probably to either pass an in memory storage or a noop storage.

This is what I've done, so I'm good since you've stated that this is the purpose of the use of localStorage. I'm still prototyping to decide if I can/want to use Supabase, so it is quite possible I will end up putting in some kind of alternative implementation to serve the same purpose.

If you are using Supabase Auth on a non-browser environment where there's no localStorage you need to provide an alternative storage provider using the storage option.

Unfortunately, to keep compatibility with many environments we'd rather have this code crash and make it apparent that you need to specify a storage provider instead of failing silently or using RAM-based storage.

Do continue the discussion or re-open the issue if you feel I've not considered all points.

Well, I gave up trying Supabase in the end, so this is ok by me. Still, I am little surprised by your decision since it seems to be the objective of the persistSession variable to avoid the use of (and therefore need of) localStorage. Anyway, your call.