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

Unguarded localStorage access causes uncaught crashes when dealing with auth #3312

Closed
tmcw opened this issue Apr 24, 2022 · 2 comments
Closed
Labels
kind/bug Something isn't working status/done

Comments

@tmcw
Copy link
Contributor

tmcw commented Apr 24, 2022

What is the problem?

People using browsers with security-paranoid configurations have some unusual localStorage behavior. You either get:

  • Chrome on Android in private mode or with paranoid extensions = no localStorage object at all. localStorage is null.
  • Firefox in private mode: localStorage exists, but if you touch it, you get an error

So: the essence is that Blitz's auth system assumes that localStorage both exists and works, and that is not a safe assumption. In production, this leads to uncaught errors.

This was reported in blitz-js/legacy-framework#111, but only touched one instance. There are several more localStorage accesses.

Paste all your error logs here:

TypeError: Cannot read properties of null (reading 'getItem')
  at getToken(./node_modules/next/dist/data-client/auth.js:97:33)
  at getData(./node_modules/next/dist/data-client/auth.js:84:38)
  at updateState(./node_modules/next/dist/data-client/auth.js:76:80)
  at new h(./node_modules/next/dist/data-client/auth.js:105:18)
  at getPublicDataStore(./node_modules/next/dist/data-client/auth.js:121:36)
  at c(./node_modules/next/dist/stdlib/blitz-app-root.js:109:43)
  at Nh(./node_modules/react-dom/cjs/react-dom.production.min.js:166:137)
  at Lk(./node_modules/react-dom/cjs/react-dom.production.min.js:289:386)
  at Kk(./node_modules/react-dom/cjs/react-dom.production.min.js:279:379)
  at Jk(./node_modules/react-dom/cjs/react-dom.production.min.js:279:310)
  at yk(./node_modules/react-dom/cjs/react-dom.production.min.js:279:170)
  at Dk(./node_modules/react-dom/cjs/react-dom.production.min.js:270:443)
  at wk(./node_modules/react-dom/cjs/react-dom.production.min.js:268:421)
  at J(./node_modules/react-dom/node_modules/scheduler/cjs/scheduler.production.min.js:13:203)
  at MessagePort.T(./node_modules/react-dom/node_modules/scheduler/cjs/scheduler.production.min.js:14:128)

Paste all relevant code snippets here:

(this is part of the default flow with Blitz)

What are detailed steps to reproduce this?

  1. Use Firefox in private mode or Chrome in private mode on Android

Run blitz -v and paste the output here:

blitz: 0.45.4 (local)

  Package manager: yarn
  System:
    OS: macOS 12.3.1
    CPU: (16) x64 Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
    Memory: 66.32 MB / 16.00 GB
    Shell: 5.8.1 - /usr/local/bin/zsh
  Binaries:
    Node: 16.13.1 - ~/n/bin/node
    Yarn: 1.22.4 - ~/.yarn/bin/yarn
    npm: 6.14.15 - /usr/local/bin/npm
    Watchman: Not Found
  npmPackages:
    @prisma/client: ~3.12 => 3.12.0
    blitz: 0.45.4 => 0.45.4
    prisma: ~3.12 => 3.12.0
    react: ^18.0.0 => 18.0.0
    react-dom: ^18.0.0 => 18.0.0
    typescript: ~4.6 => 4.6.3

Please include below any other applicable logs and screenshots that show your problem:

No response

@tmcw tmcw added kind/bug Something isn't working status/triage labels Apr 24, 2022
@beerose
Copy link
Contributor

beerose commented Jun 22, 2022

Thanks for the issue. What would be the desired behaviour if we get rid of the assumption that LS exists?

@tmcw
Copy link
Contributor Author

tmcw commented Jun 22, 2022

Thanks! I think that desired behavior would be catching exceptions around localStorage access, and treating this crash the same way you'd treat the key as not existing in localStorage.

It's a sort of rare configuration and one that honestly blows up a lot of websites anyway, but crashes from this bubble up to the application layer, which is annoying.

@beerose beerose added the status/ready-to-work-on This issue is up for grabs label Jun 22, 2022
@blitzjs-bot blitzjs-bot added status/done and removed status/ready-to-work-on This issue is up for grabs labels Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working status/done
Projects
None yet
Development

No branches or pull requests

4 participants