From 5faa098b3d9cd30d400a44e6724af9b1ba1e1fdc Mon Sep 17 00:00:00 2001 From: Sergey Nikitin Date: Wed, 31 Jan 2024 21:03:02 +0100 Subject: [PATCH] fix: proxy localStorage and sessionStorage (closes #293) In the current implementation, we cache both storages (localStorage and sessionStorage) at the moment of worker creation. Although this could be considered as performance optimization, it leads to the implementation inconsistency: - Storages data leaks into 3rd party script - Storages easily go out of sync as there is no proper synchronization with main window setup. - One can not override the storage access easily. For the latter point, this means we can not take on the methods like `localStorage.getItem()` in the Partytown proxy methods (specifically in `apply`). Hence there is no way to shield the storage data from 3rd party scripts. In this commit we fully rely on the storage values the main window sends into the worker. As the result, we can always receive relevant value and also can override the storage api with `apply` and `get` hooks --- src/lib/log.ts | 4 +- src/lib/sandbox/read-main-platform.ts | 18 +- src/lib/types.ts | 4 - src/lib/web-worker/init-web-worker.ts | 5 - src/lib/web-worker/worker-constants.ts | 5 - src/lib/web-worker/worker-document.ts | 6 +- src/lib/web-worker/worker-storage.ts | 64 ++--- src/lib/web-worker/worker-window.ts | 6 +- tests/platform/storage/storage-access.html | 230 ++++++++++++++++++ tests/platform/storage/storage-access.spec.ts | 33 +++ 10 files changed, 295 insertions(+), 80 deletions(-) create mode 100644 tests/platform/storage/storage-access.html create mode 100644 tests/platform/storage/storage-access.spec.ts diff --git a/src/lib/log.ts b/src/lib/log.ts index d0149a5c..bcf1f31c 100644 --- a/src/lib/log.ts +++ b/src/lib/log.ts @@ -16,8 +16,8 @@ import { } from './web-worker/worker-constants'; import { debug, getConstructorName, isPromise } from './utils'; -export const warnCrossOrgin = ( - apiType: 'get' | 'set' | 'remove' | 'clear', +export const warnCrossOrigin = ( + apiType: 'get' | 'set' | 'remove' | 'clear' | 'length' | 'key', apiName: string, env: WebWorkerEnvironment ) => console.warn(`Partytown unable to ${apiType} cross-origin ${apiName}: ` + env.$location$); diff --git a/src/lib/sandbox/read-main-platform.ts b/src/lib/sandbox/read-main-platform.ts index 1293907d..832a6762 100644 --- a/src/lib/sandbox/read-main-platform.ts +++ b/src/lib/sandbox/read-main-platform.ts @@ -3,7 +3,6 @@ import { getConstructorName, getNodeName, isValidMemberName, - len, noop, serializeConfig, } from '../utils'; @@ -13,7 +12,6 @@ import { InterfaceInfo, InterfaceMember, InitWebWorkerData, - StorageItem, } from '../types'; export const readMainPlatform = () => { @@ -68,9 +66,7 @@ export const readMainPlatform = () => { $config$, $interfaces$: readImplementations(impls, initialInterfaces), $libPath$: new URL(libPath, mainWindow.location as any) + '', - $origin$: origin, - $localStorage$: readStorage('localStorage'), - $sessionStorage$: readStorage('sessionStorage'), + $origin$: origin }; addGlobalConstructorUsingPrototype( @@ -186,18 +182,6 @@ const readImplementationMember = ( } }; -const readStorage = (storageName: 'localStorage' | 'sessionStorage') => { - let items: StorageItem[] = []; - let i = 0; - let l = len(mainWindow[storageName]); - let key: string; - for (; i < l; i++) { - key = mainWindow[storageName].key(i)!; - items.push([key, mainWindow[storageName].getItem(key)!]); - } - return items; -}; - const getGlobalConstructor = (mainWindow: any, cstrName: string) => typeof mainWindow[cstrName] !== 'undefined' ? new mainWindow[cstrName](noop) : 0; diff --git a/src/lib/types.ts b/src/lib/types.ts index 8e0a3305..3b6b22ed 100644 --- a/src/lib/types.ts +++ b/src/lib/types.ts @@ -127,8 +127,6 @@ export interface InitWebWorkerData { $interfaces$: InterfaceInfo[]; $libPath$: string; $sharedDataBuffer$?: SharedArrayBuffer; - $localStorage$: StorageItem[]; - $sessionStorage$: StorageItem[]; $origin$: string; } @@ -639,8 +637,6 @@ export type StateMap = Record; export type StateRecord = Record; -export type StorageItem = [/*key*/ string, /*value*/ string]; - export const enum CallType { Blocking = 1, NonBlocking = 2, diff --git a/src/lib/web-worker/init-web-worker.ts b/src/lib/web-worker/init-web-worker.ts index 801d160b..5d9320bb 100644 --- a/src/lib/web-worker/init-web-worker.ts +++ b/src/lib/web-worker/init-web-worker.ts @@ -1,8 +1,6 @@ import { commaSplit, webWorkerCtx, - webWorkerlocalStorage, - webWorkerSessionStorage, } from './worker-constants'; import type { InitWebWorkerData, PartytownInternalConfig } from '../types'; @@ -18,9 +16,6 @@ export const initWebWorker = (initWebWorkerData: InitWebWorkerData) => { webWorkerCtx.$postMessage$ = (postMessage as any).bind(self); webWorkerCtx.$sharedDataBuffer$ = initWebWorkerData.$sharedDataBuffer$; - webWorkerlocalStorage.set(locOrigin, initWebWorkerData.$localStorage$); - webWorkerSessionStorage.set(locOrigin, initWebWorkerData.$sessionStorage$); - (self as any).importScripts = undefined; delete (self as any).postMessage; delete (self as any).WorkerGlobalScope; diff --git a/src/lib/web-worker/worker-constants.ts b/src/lib/web-worker/worker-constants.ts index 10d77064..eef430c9 100644 --- a/src/lib/web-worker/worker-constants.ts +++ b/src/lib/web-worker/worker-constants.ts @@ -4,7 +4,6 @@ import type { PostMessageData, RefHandler, RefId, - StorageItem, WebWorkerContext, WebWorkerEnvironment, WinId, @@ -27,10 +26,6 @@ export const postMessages: PostMessageData[] = []; export const webWorkerCtx: WebWorkerContext = {} as any; export const webWorkerInterfaces: InterfaceInfo[] = []; - -export const webWorkerlocalStorage = /*#__PURE__*/ new Map(); -export const webWorkerSessionStorage = /*#__PURE__*/ new Map(); - export const environments: { [winId: WinId]: WebWorkerEnvironment } = {}; export const cachedDimensions = /*#__PURE__*/ new Map(); diff --git a/src/lib/web-worker/worker-document.ts b/src/lib/web-worker/worker-document.ts index f05d4414..cb4eac1a 100644 --- a/src/lib/web-worker/worker-document.ts +++ b/src/lib/web-worker/worker-document.ts @@ -20,7 +20,7 @@ import { ABOUT_BLANK, elementStructurePropNames, IS_TAG_REG, WinIdKey } from './ import { getInstanceStateValue } from './worker-state'; import { getPartytownScript } from './worker-exec'; import { isScriptJsType } from './worker-script'; -import { warnCrossOrgin } from '../log'; +import { warnCrossOrigin } from '../log'; export const patchDocument = ( WorkerDocument: any, @@ -39,7 +39,7 @@ export const patchDocument = ( if (env.$isSameOrigin$) { return getter(this, ['cookie']); } else { - warnCrossOrgin('get', 'cookie', env); + warnCrossOrigin('get', 'cookie', env); return ''; } }, @@ -47,7 +47,7 @@ export const patchDocument = ( if (env.$isSameOrigin$) { setter(this, ['cookie'], value); } else if (debug) { - warnCrossOrgin('set', 'cookie', env); + warnCrossOrigin('set', 'cookie', env); } }, }, diff --git a/src/lib/web-worker/worker-storage.ts b/src/lib/web-worker/worker-storage.ts index 70d07098..67ac082b 100644 --- a/src/lib/web-worker/worker-storage.ts +++ b/src/lib/web-worker/worker-storage.ts @@ -1,74 +1,61 @@ -import { callMethod } from './worker-proxy'; -import { CallType, StorageItem, WebWorkerEnvironment } from '../types'; +import { callMethod, getter } from './worker-proxy'; +import { CallType, WebWorkerEnvironment } from '../types'; import { EMPTY_ARRAY } from '../utils'; -import { warnCrossOrgin } from '../log'; +import { warnCrossOrigin } from '../log'; export const addStorageApi = ( win: any, storageName: 'localStorage' | 'sessionStorage', - storages: Map, isSameOrigin: boolean, env: WebWorkerEnvironment ) => { - let getItems = (items?: StorageItem[]) => { - items = storages.get(win.origin); - if (!items) { - storages.set(win.origin, (items = [])); - } - return items; - }; - let getIndexByKey = (key: string) => getItems().findIndex((i) => i[STORAGE_KEY] === key); - let index: number; - let item: StorageItem; - let storage: Storage = { getItem(key) { - index = getIndexByKey(key); - return index > -1 ? getItems()[index][STORAGE_VALUE] : null; + if (isSameOrigin) { + return callMethod(win, [storageName, 'getItem'], [key], CallType.Blocking); + } else { + warnCrossOrigin('get', storageName, env); + } }, setItem(key, value) { - index = getIndexByKey(key); - if (index > -1) { - getItems()[index][STORAGE_VALUE] = value; - } else { - getItems().push([key, value]); - } if (isSameOrigin) { - callMethod(win, [storageName, 'setItem'], [key, value], CallType.NonBlocking); + callMethod(win, [storageName, 'setItem'], [key, value], CallType.Blocking); } else { - warnCrossOrgin('set', storageName, env); + warnCrossOrigin('set', storageName, env); } }, removeItem(key) { - index = getIndexByKey(key); - if (index > -1) { - getItems().splice(index, 1); - } if (isSameOrigin) { - callMethod(win, [storageName, 'removeItem'], [key], CallType.NonBlocking); + callMethod(win, [storageName, 'removeItem'], [key], CallType.Blocking); } else { - warnCrossOrgin('remove', storageName, env); + warnCrossOrigin('remove', storageName, env); } }, key(index) { - item = getItems()[index]; - return item ? item[STORAGE_KEY] : null; + if (isSameOrigin) { + return callMethod(win, [storageName, 'key'], [index], CallType.Blocking); + } else { + warnCrossOrigin('key', storageName, env); + } }, clear() { - getItems().length = 0; if (isSameOrigin) { - callMethod(win, [storageName, 'clear'], EMPTY_ARRAY, CallType.NonBlocking); + callMethod(win, [storageName, 'clear'], EMPTY_ARRAY, CallType.Blocking); } else { - warnCrossOrgin('clear', storageName, env); + warnCrossOrigin('clear', storageName, env); } }, get length() { - return getItems().length; + if (isSameOrigin) { + return getter(win, [storageName, 'length']) + } else { + warnCrossOrigin('length', storageName, env); + } }, }; @@ -99,6 +86,3 @@ export const addStorageApi = ( }, }); }; - -const STORAGE_KEY = 0; -const STORAGE_VALUE = 1; diff --git a/src/lib/web-worker/worker-window.ts b/src/lib/web-worker/worker-window.ts index 21230a62..594655b7 100644 --- a/src/lib/web-worker/worker-window.ts +++ b/src/lib/web-worker/worker-window.ts @@ -25,8 +25,6 @@ import { NamespaceKey, postMessages, webWorkerCtx, - webWorkerlocalStorage, - webWorkerSessionStorage, WinIdKey, } from './worker-constants'; import { createCustomElementRegistry } from './worker-custom-elements'; @@ -419,8 +417,8 @@ export const createWindow = ( win.cancelIdleCallback = (id: number) => clearTimeout(id); // add storage APIs to the window - addStorageApi(win, 'localStorage', webWorkerlocalStorage, $isSameOrigin$, env); - addStorageApi(win, 'sessionStorage', webWorkerSessionStorage, $isSameOrigin$, env); + addStorageApi(win, 'localStorage', $isSameOrigin$, env); + addStorageApi(win, 'sessionStorage', $isSameOrigin$, env); if (!$isSameOrigin$) { win.indexeddb = undefined; diff --git a/tests/platform/storage/storage-access.html b/tests/platform/storage/storage-access.html new file mode 100644 index 00000000..60cca6ac --- /dev/null +++ b/tests/platform/storage/storage-access.html @@ -0,0 +1,230 @@ + + + + + + + Storage + + + + + + +

Storage

+
    +
  • + Local storage size + + +
    • + +
  • + Local storage getItem + + +
    • + +
  • + Local storage setItem + +
    • + +
  • + Local storage removeItem + +
    • + +
  • + Local storage clear + +
    • + +
  • + Local storage key + + +
    • + +
  • + Session storage size + + +
    • + +
  • + Session storage getItem + + +
    • + +
  • + Session storage setItem + +
    • + +
  • + Session storage removeItem + +
    • + +
  • + Session storage clear + +
    • + +
  • + Session storage key + + +
    • + + +
+ +
+

All Tests

+ + diff --git a/tests/platform/storage/storage-access.spec.ts b/tests/platform/storage/storage-access.spec.ts new file mode 100644 index 00000000..52c5687e --- /dev/null +++ b/tests/platform/storage/storage-access.spec.ts @@ -0,0 +1,33 @@ +import { test, expect } from '@playwright/test'; + +test('storage access', async ({ page }) => { + await page.goto('/tests/platform/storage/storage-access.html'); + + await page.waitForSelector('.completed'); + + const testLocalStorageSize = page.locator('#localStorageSize'); + await expect(testLocalStorageSize).toHaveText('13'); + + const testLocalStorageGetItem = page.locator('#localStorageItem'); + await expect(testLocalStorageGetItem).toHaveText('mocked localStorage.getItem'); + + // @ts-expect-error localStorage exists on the page + const realLocalStorageItem = await page.evaluate(() => localStorage.getItem('dmc')) + expect(realLocalStorageItem).toBe('12'); + + const testLocalStorageKey = page.locator('#localStorageKey'); + await expect(testLocalStorageKey).toHaveText('mocked localStorage.key'); + + const testSessionStorageSize = page.locator('#sessionStorageSize'); + await expect(testSessionStorageSize).toHaveText('10'); + + const testSessionStorageGetItem = page.locator('#sessionStorageItem'); + await expect(testSessionStorageGetItem).toHaveText('mocked sessionStorage.getItem'); + + // @ts-expect-error sessionStorage exists on the page + const realSessionStorageItem = await page.evaluate(() => sessionStorage.getItem('mph')) + expect(realSessionStorageItem).toBe('88'); + + const testSessionStorageKey = page.locator('#sessionStorageKey'); + await expect(testSessionStorageKey).toHaveText('mocked sessionStorage.key'); +});