From 29a7eadb8531c7c83cee3e9631bfc56651ac3622 Mon Sep 17 00:00:00 2001 From: Dmitry S Date: Wed, 30 Nov 2022 10:55:47 -0500 Subject: [PATCH] (core) Fix problem with localStorage in some cross-origin embed situations Summary: - Handle the possibility that any access to localStorage causes error. - Move getStorage() and getSessionStorage() safe functions to a separate file. - Use these safe functions in more places. Test Plan: Added a test case, using a webdriver instance that blocks third-party cookies, to enforce third-party restrictions. Added to gristUtil a way to override the webdriver instance. Reviewers: jarek Reviewed By: jarek Differential Revision: https://phab.getgrist.com/D3719 --- app/client/components/CursorMonitor.ts | 2 +- app/client/components/EditorMonitor.ts | 2 +- app/client/components/GristWSConnection.ts | 11 ++-- app/client/lib/localStorageObs.ts | 64 +-------------------- app/client/lib/sessionObs.ts | 8 ++- app/client/lib/storage.ts | 65 ++++++++++++++++++++++ app/client/ui2018/cssVars.ts | 3 +- test/nbrowser/gristUtils.ts | 13 ++++- 8 files changed, 93 insertions(+), 75 deletions(-) create mode 100644 app/client/lib/storage.ts diff --git a/app/client/components/CursorMonitor.ts b/app/client/components/CursorMonitor.ts index 408b3c7c..0d83aa5d 100644 --- a/app/client/components/CursorMonitor.ts +++ b/app/client/components/CursorMonitor.ts @@ -1,6 +1,6 @@ import {CursorPos} from 'app/client/components/Cursor'; import {GristDoc} from 'app/client/components/GristDoc'; -import {getStorage} from 'app/client/lib/localStorageObs'; +import {getStorage} from 'app/client/lib/storage'; import {IDocPage, isViewDocPage, ViewDocPage} from 'app/common/gristUrls'; import {Disposable, Listener, Observable} from 'grainjs'; import {reportError} from 'app/client/models/errors'; diff --git a/app/client/components/EditorMonitor.ts b/app/client/components/EditorMonitor.ts index e46fb870..56f4f801 100644 --- a/app/client/components/EditorMonitor.ts +++ b/app/client/components/EditorMonitor.ts @@ -1,7 +1,7 @@ import {CellPosition, toCursor} from 'app/client/components/CellPosition'; import {oneTimeListener} from 'app/client/components/CursorMonitor'; import {GristDoc} from 'app/client/components/GristDoc'; -import {getStorage} from 'app/client/lib/localStorageObs'; +import {getStorage} from 'app/client/lib/storage'; import {UserError} from 'app/client/models/errors'; import {FieldEditor, FieldEditorStateEvent} from 'app/client/widgets/FieldEditor'; import {isViewDocPage} from 'app/common/gristUrls'; diff --git a/app/client/components/GristWSConnection.ts b/app/client/components/GristWSConnection.ts index 5410a9d6..5a465412 100644 --- a/app/client/components/GristWSConnection.ts +++ b/app/client/components/GristWSConnection.ts @@ -1,5 +1,6 @@ import {get as getBrowserGlobals} from 'app/client/lib/browserGlobals'; import {guessTimezone} from 'app/client/lib/guessTimezone'; +import {getSessionStorage} from 'app/client/lib/storage'; import {getWorker} from 'app/client/models/gristConfigCache'; import {CommResponseBase} from 'app/common/CommTypes'; import * as gutil from 'app/common/gutil'; @@ -70,6 +71,8 @@ export interface GristWSSettings { * An implementation of Grist websocket connection settings for the browser. */ export class GristWSSettingsBrowser implements GristWSSettings { + private _sessionStorage = getSessionStorage(); + public makeWebSocket(url: string) { return new WebSocket(url); } public getTimezone() { return guessTimezone(); } public getPageUrl() { return G.window.location.href; } @@ -77,18 +80,18 @@ export class GristWSSettingsBrowser implements GristWSSettings { return getDocWorkerUrl(assignmentId); } public getClientId(assignmentId: string|null) { - return window.sessionStorage.getItem(`clientId_${assignmentId}`) || null; + return this._sessionStorage.getItem(`clientId_${assignmentId}`) || null; } public getUserSelector(): string { // TODO: find/create a more official way to get the user. return (window as any).gristDocPageModel?.appModel.currentUser?.email || ''; } public updateClientId(assignmentId: string|null, id: string) { - window.sessionStorage.setItem(`clientId_${assignmentId}`, id); + this._sessionStorage.setItem(`clientId_${assignmentId}`, id); } public advanceCounter(): string { - const value = parseInt(window.sessionStorage.getItem('clientCounter')!, 10) || 0; - window.sessionStorage.setItem('clientCounter', String(value + 1)); + const value = parseInt(this._sessionStorage.getItem('clientCounter')!, 10) || 0; + this._sessionStorage.setItem('clientCounter', String(value + 1)); return String(value); } public log(...args: any[]): void { diff --git a/app/client/lib/localStorageObs.ts b/app/client/lib/localStorageObs.ts index d4a537c6..05a36c20 100644 --- a/app/client/lib/localStorageObs.ts +++ b/app/client/lib/localStorageObs.ts @@ -1,68 +1,6 @@ import {safeJsonParse} from 'app/common/gutil'; import {Observable} from 'grainjs'; - -/** - * Returns true if storage is functional. In some cass (e.g. when embedded), localStorage may - * throw errors. If so, we return false. This implementation is the approach taken by store.js. - */ -function testStorage(storage: Storage) { - try { - const testStr = '__localStorage_test'; - storage.setItem(testStr, testStr); - const ok = (storage.getItem(testStr) === testStr); - storage.removeItem(testStr); - return ok; - } catch (e) { - return false; - } -} - -/** - * Returns localStorage if functional, or sessionStorage, or an in-memory storage. The fallbacks - * help with tests, and may help when Grist is embedded. - */ -export function getStorage(): Storage { - return _storage || (_storage = createStorage()); -} - -/** - * Similar to `getStorage`, but always returns sessionStorage (or an in-memory equivalent). - */ -export function getSessionStorage(): Storage { - return _sessionStorage || (_sessionStorage = createSessionStorage()); -} - -let _storage: Storage|undefined; -let _sessionStorage: Storage|undefined; - -function createStorage(): Storage { - if (typeof localStorage !== 'undefined' && testStorage(localStorage)) { - return localStorage; - } else { - return createSessionStorage(); - } -} - -function createSessionStorage(): Storage { - if (typeof sessionStorage !== 'undefined' && testStorage(sessionStorage)) { - return sessionStorage; - } else { - // Fall back to a Map-based implementation of (non-persistent) sessionStorage. - return createInMemoryStorage(); - } -} - -function createInMemoryStorage(): Storage { - const values = new Map(); - return { - setItem(key: string, val: string) { values.set(key, val); }, - getItem(key: string) { return values.get(key) ?? null; }, - removeItem(key: string) { values.delete(key); }, - clear() { values.clear(); }, - get length() { return values.size; }, - key(index: number): string|null { throw new Error('Not implemented'); }, - }; -} +import {getSessionStorage, getStorage} from 'app/client/lib/storage'; function getStorageBoolObs(store: Storage, key: string, defValue: boolean) { const storedNegation = defValue ? 'false' : 'true'; diff --git a/app/client/lib/sessionObs.ts b/app/client/lib/sessionObs.ts index 3c23c93d..e16b5b2f 100644 --- a/app/client/lib/sessionObs.ts +++ b/app/client/lib/sessionObs.ts @@ -4,6 +4,7 @@ */ import {safeJsonParse} from 'app/common/gutil'; import {IDisposableOwner, Observable} from 'grainjs'; +import {getSessionStorage} from 'app/client/lib/storage'; export interface SessionObs extends Observable { pauseSaving(yesNo: boolean): void; @@ -45,14 +46,15 @@ export function createSessionObs( return value === _default || !isValid(value) ? null : JSON.stringify(value); } let _pauseSaving = false; - const obs = Observable.create(owner, fromString(window.sessionStorage.getItem(key))); + const storage = getSessionStorage(); + const obs = Observable.create(owner, fromString(storage.getItem(key))); obs.addListener((value: T) => { if (_pauseSaving) { return; } const stored = toString(value); if (stored == null) { - window.sessionStorage.removeItem(key); + storage.removeItem(key); } else { - window.sessionStorage.setItem(key, stored); + storage.setItem(key, stored); } }); return Object.assign(obs, {pauseSaving(yesNo: boolean) { _pauseSaving = yesNo; }}); diff --git a/app/client/lib/storage.ts b/app/client/lib/storage.ts new file mode 100644 index 00000000..6d859109 --- /dev/null +++ b/app/client/lib/storage.ts @@ -0,0 +1,65 @@ +/** + * Expose localStorage and sessionStorage with fallbacks for cases when they don't work (e.g. + * cross-domain embeds in Firefox and Safari). + * + * Usage: + * import {getStorage, getSessionStorage} from 'app/client/lib/storage'; + * ... use getStorage() in place of localStorage... + * ... use getSessionStorage() in place of sessionStorage... + */ + +/** + * Returns localStorage if functional, or sessionStorage, or an in-memory storage. The fallbacks + * help with tests, and when Grist is embedded. + */ +export function getStorage(): Storage { + _storage ??= testStorage('localStorage') || getSessionStorage(); + return _storage; +} + +/** + * Return window.sessionStorage, or when not available, an in-memory storage. + */ +export function getSessionStorage(): Storage { + // If can't use sessionStorage, fall back to a Map-based non-persistent implementation. + _sessionStorage ??= testStorage('sessionStorage') || createInMemoryStorage(); + return _sessionStorage; +} + + +let _storage: Storage|undefined; +let _sessionStorage: Storage|undefined; + +/** + * Returns the result of window[storageName] if storage is functional, or null otherwise. In some + * cases (e.g. when embedded), using localStorage may throw errors, in which case we return null. + * This is similar to the approach taken by store.js. + */ +function testStorage(storageName: 'localStorage'|'sessionStorage'): Storage|null { + try { + const testStr = '__localStorage_test'; + const storage = window[storageName]; + storage.setItem(testStr, testStr); + const ok = (storage.getItem(testStr) === testStr); + storage.removeItem(testStr); + if (ok) { + return storage; + } + } catch (e) { + // Fall through + } + console.warn(`${storageName} is not available; will use fallback`); + return null; +} + +function createInMemoryStorage(): Storage { + const values = new Map(); + return { + setItem(key: string, val: string) { values.set(key, val); }, + getItem(key: string) { return values.get(key) ?? null; }, + removeItem(key: string) { values.delete(key); }, + clear() { values.clear(); }, + get length() { return values.size; }, + key(index: number): string|null { throw new Error('Not implemented'); }, + }; +} diff --git a/app/client/ui2018/cssVars.ts b/app/client/ui2018/cssVars.ts index 45203e60..fb5a5069 100644 --- a/app/client/ui2018/cssVars.ts +++ b/app/client/ui2018/cssVars.ts @@ -6,6 +6,7 @@ * https://css-tricks.com/snippets/css/system-font-stack/ * */ +import {getStorage} from 'app/client/lib/storage'; import {urlState} from 'app/client/models/gristUrlState'; import {getTheme, ProductFlavor} from 'app/client/ui/CustomThemes'; import {Theme, ThemeAppearance} from 'app/common/ThemePrefs'; @@ -838,7 +839,7 @@ ${properties.join('\n')} // Cache the appearance in local storage; this is currently used to apply a suitable // background image that's shown while the application is loading. - localStorage.setItem('appearance', appearance); + getStorage().setItem('appearance', appearance); } /** diff --git a/test/nbrowser/gristUtils.ts b/test/nbrowser/gristUtils.ts index c83278e4..16388b7a 100644 --- a/test/nbrowser/gristUtils.ts +++ b/test/nbrowser/gristUtils.ts @@ -7,8 +7,8 @@ import * as fse from 'fs-extra'; import escapeRegExp = require('lodash/escapeRegExp'); import noop = require('lodash/noop'); import startCase = require('lodash/startCase'); -import { assert, driver, error, Key, WebElement, WebElementPromise } from 'mocha-webdriver'; -import { stackWrapFunc, stackWrapOwnMethods } from 'mocha-webdriver'; +import { assert, driver as driverOrig, error, Key, WebElement, WebElementPromise } from 'mocha-webdriver'; +import { stackWrapFunc, stackWrapOwnMethods, WebDriver } from 'mocha-webdriver'; import * as path from 'path'; import { decodeUrl } from 'app/common/gristUrls'; @@ -32,6 +32,15 @@ import type { AssertionError } from 'assert'; // Wrap in a namespace so that we can apply stackWrapOwnMethods to all the exports together. namespace gristUtils { +// Allow overriding the global 'driver' to use in gristUtil. +let driver: WebDriver; + +// Substitute a custom driver to use with gristUtils functions. Omit argument to restore to default. +export function setDriver(customDriver: WebDriver = driverOrig) { driver = customDriver; } + +// Set the 'driver' to use here in before() callback, because driverOrig isn't set until then. +before(() => setDriver()); + const homeUtil = new HomeUtil(testUtils.fixturesRoot, server); export const createNewDoc = homeUtil.createNewDoc.bind(homeUtil);