From d635e1bcac9828204d29bf785d292fb24f5bc2f1 Mon Sep 17 00:00:00 2001 From: Danil Somsikov Date: Thu, 12 Jan 2023 09:42:04 +0100 Subject: [PATCH] Remove security origin support from DOM Storage code as it was buggy and unnecessary. Bug: 1347831 Change-Id: Icf1c52bc8f765123b473c2cba738f88f6f552b63 Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/4127160 Commit-Queue: Danil Somsikov Auto-Submit: Danil Somsikov Reviewed-by: Kateryna Prokopenko --- .../application/ApplicationPanelSidebar.ts | 12 +- .../panels/application/DOMStorageModel.ts | 156 ++---------------- front_end/panels/application/StorageView.ts | 50 +----- .../application/DOMStorageModel_test.ts | 13 +- .../panels/application/StorageView_test.ts | 20 +-- 5 files changed, 32 insertions(+), 219 deletions(-) diff --git a/front_end/panels/application/ApplicationPanelSidebar.ts b/front_end/panels/application/ApplicationPanelSidebar.ts index 1e010aa8cc..f896a0591b 100644 --- a/front_end/panels/application/ApplicationPanelSidebar.ts +++ b/front_end/panels/application/ApplicationPanelSidebar.ts @@ -729,7 +729,7 @@ export class ApplicationPanelSidebar extends UI.Widget.VBox implements SDK.Targe private addDOMStorage(domStorage: DOMStorage): void { console.assert(!this.domStorageTreeElements.get(domStorage)); - console.assert(Boolean(domStorage.storageKey) || Boolean(domStorage.securityOrigin)); + console.assert(Boolean(domStorage.storageKey)); const domStorageTreeElement = new DOMStorageTreeElement(this.panel, domStorage); this.domStorageTreeElements.set(domStorage, domStorageTreeElement); @@ -1665,19 +1665,15 @@ export class IDBIndexTreeElement extends ApplicationPanelTreeElement { export class DOMStorageTreeElement extends ApplicationPanelTreeElement { private readonly domStorage: DOMStorage; constructor(storagePanel: ResourcesPanel, domStorage: DOMStorage) { - super( - storagePanel, - domStorage.securityOrigin ? domStorage.securityOrigin : - (domStorage.storageKey ? domStorage.storageKey : i18nString(UIStrings.localFiles)), - false); + super(storagePanel, domStorage.storageKey ? domStorage.storageKey : i18nString(UIStrings.localFiles), false); this.domStorage = domStorage; const icon = UI.Icon.Icon.create('mediumicon-table', 'resource-tree-item'); this.setLeadingIcons([icon]); } get itemURL(): Platform.DevToolsPath.UrlString { - return 'storage://' + this.domStorage.securityOrigin + '/' + - (this.domStorage.isLocalStorage ? 'local' : 'session') as Platform.DevToolsPath.UrlString; + return 'storage://' + this.domStorage.storageKey + '/' + (this.domStorage.isLocalStorage ? 'local' : 'session') as + Platform.DevToolsPath.UrlString; } onselect(selectedByUser?: boolean): boolean { diff --git a/front_end/panels/application/DOMStorageModel.ts b/front_end/panels/application/DOMStorageModel.ts index 88500a0aa8..41522f60be 100644 --- a/front_end/panels/application/DOMStorageModel.ts +++ b/front_end/panels/application/DOMStorageModel.ts @@ -38,55 +38,22 @@ import type * as ProtocolProxyApi from '../../generated/protocol-proxy-api.js'; export class DOMStorage extends Common.ObjectWrapper.ObjectWrapper { private readonly model: DOMStorageModel; - private readonly securityOriginInternal: string|null; - private readonly storageKeyInternal: string|null; + private readonly storageKeyInternal: string; private readonly isLocalStorageInternal: boolean; - constructor(model: DOMStorageModel, securityOrigin: string, storageKey: string, isLocalStorage: boolean) { + constructor(model: DOMStorageModel, storageKey: string, isLocalStorage: boolean) { super(); this.model = model; - this.securityOriginInternal = securityOrigin; this.storageKeyInternal = storageKey; this.isLocalStorageInternal = isLocalStorage; } - static storageId(securityOrigin: string, isLocalStorage: boolean): Protocol.DOMStorage.StorageId { - return {securityOrigin: securityOrigin, isLocalStorage: isLocalStorage}; - } - - static storageIdWithSecurityOrigin(securityOrigin: string, isLocalStorage: boolean): Protocol.DOMStorage.StorageId { - return {securityOrigin: securityOrigin, isLocalStorage: isLocalStorage}; - } - - static storageIdWithStorageKey(storageKey: string, isLocalStorage: boolean): Protocol.DOMStorage.StorageId { + static storageId(storageKey: string, isLocalStorage: boolean): Protocol.DOMStorage.StorageId { return {storageKey: storageKey, isLocalStorage: isLocalStorage}; } - get idWithSecurityOrigin(): Protocol.DOMStorage.StorageId { - let securityOrigin = ''; - if (this.securityOriginInternal) { - securityOrigin = this.securityOriginInternal; - } - return DOMStorage.storageIdWithSecurityOrigin(securityOrigin, this.isLocalStorageInternal); - } - - get idWithStorageKey(): Protocol.DOMStorage.StorageId { - let storageKey = ''; - if (this.storageKeyInternal) { - storageKey = this.storageKeyInternal; - } - return DOMStorage.storageIdWithStorageKey(storageKey, this.isLocalStorageInternal); - } - get id(): Protocol.DOMStorage.StorageId { - if (this.storageKeyInternal) { - return this.idWithStorageKey; - } - return this.idWithSecurityOrigin; - } - - get securityOrigin(): string|null { - return this.securityOriginInternal; + return DOMStorage.storageId(this.storageKeyInternal, this.isLocalStorageInternal); } get storageKey(): string|null { @@ -148,7 +115,6 @@ export namespace DOMStorage { } export class DOMStorageModel extends SDK.SDKModel.SDKModel { - private readonly securityOriginManager: SDK.SecurityOriginManager.SecurityOriginManager|null; private readonly storageKeyManagerInternal: SDK.StorageKeyManager.StorageKeyManager|null; private storagesInternal: { [x: string]: DOMStorage, @@ -159,7 +125,6 @@ export class DOMStorageModel extends SDK.SDKModel.SDKModel { constructor(target: SDK.Target.Target) { super(target); - this.securityOriginManager = target.model(SDK.SecurityOriginManager.SecurityOriginManager); this.storageKeyManagerInternal = target.model(SDK.StorageKeyManager.StorageKeyManager); this.storagesInternal = {}; this.agent = target.domstorageAgent(); @@ -185,43 +150,17 @@ export class DOMStorageModel extends SDK.SDKModel.SDKModel { this.addStorageKey(storageKey); } } - if (this.securityOriginManager) { - this.securityOriginManager.addEventListener( - SDK.SecurityOriginManager.Events.SecurityOriginAdded, this.securityOriginAdded, this); - this.securityOriginManager.addEventListener( - SDK.SecurityOriginManager.Events.SecurityOriginRemoved, this.securityOriginRemoved, this); - - for (const securityOrigin of this.securityOriginManager.securityOrigins()) { - this.addOrigin(securityOrigin); - } - } void this.agent.invoke_enable(); this.enabled = true; } - clearForOrigin(origin: string): void { - if (!this.enabled) { - return; - } - for (const isLocal of [true, false]) { - const key = this.keyForSecurityOrigin(origin, isLocal); - const storage = this.storagesInternal[key]; - if (!storage) { - return; - } - storage.clear(); - } - this.removeOrigin(origin); - this.addOrigin(origin); - } - clearForStorageKey(storageKey: string): void { if (!this.enabled) { return; } for (const isLocal of [true, false]) { - const key = this.keyForStorageKey(storageKey, isLocal); + const key = this.storageKey(storageKey, isLocal); const storage = this.storagesInternal[key]; if (!storage) { return; @@ -232,83 +171,27 @@ export class DOMStorageModel extends SDK.SDKModel.SDKModel { this.addStorageKey(storageKey); } - private securityOriginAdded(event: Common.EventTarget.EventTargetEvent): void { - this.addOrigin(event.data); - } - private storageKeyAdded(event: Common.EventTarget.EventTargetEvent): void { this.addStorageKey(event.data); } - private addOrigin(securityOrigin: string): void { - const parsed = new Common.ParsedURL.ParsedURL(securityOrigin); - // These are "opaque" origins which are not supposed to support DOM storage. - if (!parsed.isValid || parsed.scheme === 'data' || parsed.scheme === 'about' || parsed.scheme === 'javascript') { - return; - } - - for (const isLocal of [true, false]) { - const key = this.keyForSecurityOrigin(securityOrigin, isLocal); - console.assert(!this.storagesInternal[key]); - if (this.duplicateExists(key)) { - continue; - } - const storage = new DOMStorage(this, securityOrigin, '', isLocal); - this.storagesInternal[key] = storage; - this.dispatchEventToListeners(Events.DOMStorageAdded, storage); - } - } - private addStorageKey(storageKey: string): void { for (const isLocal of [true, false]) { - const key = this.keyForStorageKey(storageKey, isLocal); + const key = this.storageKey(storageKey, isLocal); console.assert(!this.storagesInternal[key]); - if (this.duplicateExists(key)) { - continue; - } - const storage = new DOMStorage(this, '', storageKey, isLocal); + const storage = new DOMStorage(this, storageKey, isLocal); this.storagesInternal[key] = storage; this.dispatchEventToListeners(Events.DOMStorageAdded, storage); } } - private duplicateExists(key: string): boolean { - const parsedKey = JSON.parse(key); - for (const storageInternal in this.storagesInternal) { - const parsedStorageInternalKey = JSON.parse(storageInternal); - if (parsedKey.isLocalStorage === parsedStorageInternalKey.isLocalStorage) { - if (parsedKey.storageKey?.slice(0, -1) === parsedStorageInternalKey.securityOrigin || - parsedKey.securityOrigin === parsedStorageInternalKey.storageKey?.slice(0, -1)) { - return true; - } - } - } - return false; - } - - private securityOriginRemoved(event: Common.EventTarget.EventTargetEvent): void { - this.removeOrigin(event.data); - } - private storageKeyRemoved(event: Common.EventTarget.EventTargetEvent): void { this.removeStorageKey(event.data); } - private removeOrigin(securityOrigin: string): void { - for (const isLocal of [true, false]) { - const key = this.keyForSecurityOrigin(securityOrigin, isLocal); - const storage = this.storagesInternal[key]; - if (!storage) { - continue; - } - delete this.storagesInternal[key]; - this.dispatchEventToListeners(Events.DOMStorageRemoved, storage); - } - } - private removeStorageKey(storageKey: string): void { for (const isLocal of [true, false]) { - const key = this.keyForStorageKey(storageKey, isLocal); + const key = this.storageKey(storageKey, isLocal); const storage = this.storagesInternal[key]; if (!storage) { continue; @@ -318,23 +201,8 @@ export class DOMStorageModel extends SDK.SDKModel.SDKModel { } } - private storageKey(securityOrigin: string|undefined, storageKey: string|undefined, isLocalStorage: boolean): string { - console.assert(Boolean(securityOrigin) || Boolean(storageKey)); - if (storageKey) { - return JSON.stringify(DOMStorage.storageIdWithStorageKey(storageKey, isLocalStorage)); - } - if (securityOrigin) { - return JSON.stringify(DOMStorage.storageIdWithSecurityOrigin(securityOrigin, isLocalStorage)); - } - throw new Error('Either securityOrigin or storageKey is required'); - } - - private keyForSecurityOrigin(securityOrigin: string, isLocalStorage: boolean): string { - return this.storageKey(securityOrigin, '', isLocalStorage); - } - - private keyForStorageKey(storageKey: string, isLocalStorage: boolean): string { - return this.storageKey('', storageKey, isLocalStorage); + private storageKey(storageKey: string, isLocalStorage: boolean): string { + return JSON.stringify(DOMStorage.storageId(storageKey, isLocalStorage)); } domStorageItemsCleared(storageId: Protocol.DOMStorage.StorageId): void { @@ -377,8 +245,8 @@ export class DOMStorageModel extends SDK.SDKModel.SDKModel { } storageForId(storageId: Protocol.DOMStorage.StorageId): DOMStorage { - return this - .storagesInternal[this.storageKey(storageId.securityOrigin, storageId.storageKey, storageId.isLocalStorage)]; + console.assert(Boolean(storageId.storageKey)); + return this.storagesInternal[this.storageKey(storageId.storageKey || '', storageId.isLocalStorage)]; } storages(): DOMStorage[] { diff --git a/front_end/panels/application/StorageView.ts b/front_end/panels/application/StorageView.ts index 2684939ba5..51675de074 100644 --- a/front_end/panels/application/StorageView.ts +++ b/front_end/panels/application/StorageView.ts @@ -406,12 +406,8 @@ export class StorageView extends UI.ThrottledWidget.ThrottledWidget { if (this.target) { const includeThirdPartyCookies = this.includeThirdPartyCookiesSetting.get(); - if (this.storageKey) { - StorageView.clearByStorageKey( - this.target, this.storageKey, this.securityOrigin, selectedStorageTypes, includeThirdPartyCookies); - } else if (this.securityOrigin) { - StorageView.clear(this.target, this.securityOrigin, selectedStorageTypes, includeThirdPartyCookies); - } + StorageView.clear( + this.target, this.storageKey, this.securityOrigin, selectedStorageTypes, includeThirdPartyCookies); } this.clearButton.disabled = true; @@ -425,39 +421,12 @@ export class StorageView extends UI.ThrottledWidget.ThrottledWidget { } static clear( - target: SDK.Target.Target, securityOrigin: string, selectedStorageTypes: string[], + target: SDK.Target.Target, storageKey: string|null, originForCookies: string|null, selectedStorageTypes: string[], includeThirdPartyCookies: boolean): void { - void target.storageAgent().invoke_clearDataForOrigin( - {origin: securityOrigin, storageTypes: selectedStorageTypes.join(',')}); - - const set = new Set(selectedStorageTypes); - const hasAll = set.has(Protocol.Storage.StorageType.All); - if (set.has(Protocol.Storage.StorageType.Cookies) || hasAll) { - const cookieModel = target.model(SDK.CookieModel.CookieModel); - if (cookieModel) { - void cookieModel.clear(undefined, includeThirdPartyCookies ? undefined : securityOrigin); - } + console.assert(Boolean(storageKey)); + if (!storageKey) { + return; } - - if (set.has(Protocol.Storage.StorageType.Local_storage) || hasAll) { - const storageModel = target.model(DOMStorageModel); - if (storageModel) { - storageModel.clearForOrigin(securityOrigin); - } - } - - if (set.has(Protocol.Storage.StorageType.Websql) || hasAll) { - const databaseModel = target.model(DatabaseModel); - if (databaseModel) { - databaseModel.disable(); - databaseModel.enable(); - } - } - } - - static clearByStorageKey( - target: SDK.Target.Target, storageKey: string, originForCookies: string|undefined, selectedStorageTypes: string[], - includeThirdPartyCookies: boolean): void { void target.storageAgent().invoke_clearDataForStorageKey( {storageKey, storageTypes: selectedStorageTypes.join(',')}); @@ -637,12 +606,7 @@ export class ActionDelegate implements UI.ActionRegistration.ActionDelegate { } const securityOrigin = resourceTreeModel.getMainSecurityOrigin(); resourceTreeModel.getMainStorageKey().then(storageKey => { - if (storageKey) { - StorageView.clearByStorageKey( - target, storageKey, securityOrigin ? securityOrigin : undefined, AllStorageTypes, includeThirdPartyCookies); - } else if (securityOrigin) { - StorageView.clear(target, securityOrigin, AllStorageTypes, includeThirdPartyCookies); - } + StorageView.clear(target, storageKey, securityOrigin, AllStorageTypes, includeThirdPartyCookies); }, _ => {}); return true; } diff --git a/test/unittests/front_end/panels/application/DOMStorageModel_test.ts b/test/unittests/front_end/panels/application/DOMStorageModel_test.ts index ab408b4ec3..6fae4f9e00 100644 --- a/test/unittests/front_end/panels/application/DOMStorageModel_test.ts +++ b/test/unittests/front_end/panels/application/DOMStorageModel_test.ts @@ -20,7 +20,7 @@ describeWithMockConnection('DOMStorageModel', () => { beforeEach(() => { target = createTarget(); domStorageModel = new Resources.DOMStorageModel.DOMStorageModel(target); - domStorage = new Resources.DOMStorageModel.DOMStorage(domStorageModel, 'testOrigin', initKey, true); + domStorage = new Resources.DOMStorageModel.DOMStorage(domStorageModel, initKey, true); }); it('DOMStorage is instantiated correctly', () => { @@ -44,15 +44,4 @@ describeWithMockConnection('DOMStorageModel', () => { manager.dispatchEventToListeners(SDK.StorageKeyManager.Events.StorageKeyRemoved, testKey); assert.isUndefined(domStorageModel.storageForId(testId)); }); - - it('DOMStorage prioritises storage key over security origin if both set', () => { - const storageId = {storageKey: initKey, isLocalStorage: true}; - const getItemsSpy = sinon.spy(target.domstorageAgent(), 'invoke_getDOMStorageItems'); - - void domStorage.getItems(); - - assert.isNotEmpty(domStorage.securityOrigin); - assert.isNotEmpty(domStorage.storageKey); - assert.isTrue(getItemsSpy.calledOnceWithExactly({storageId})); - }); }); diff --git a/test/unittests/front_end/panels/application/StorageView_test.ts b/test/unittests/front_end/panels/application/StorageView_test.ts index 23600911af..ab2a1bf16b 100644 --- a/test/unittests/front_end/panels/application/StorageView_test.ts +++ b/test/unittests/front_end/panels/application/StorageView_test.ts @@ -27,7 +27,7 @@ describeWithMockConnection('StorageView', () => { storageKeyManager = target.model(SDK.StorageKeyManager.StorageKeyManager); }); - it('emits correct events on clearStorageByStorageKey', () => { + it('emits correct events on clear', () => { const testId = {storageKey: testKey, isLocalStorage: true} as Protocol.DOMStorage.StorageId; assertNotNullOrUndefined(domStorageModel); @@ -38,8 +38,7 @@ describeWithMockConnection('StorageView', () => { const dispatcherSpy = sinon.spy(domStorageModel, 'dispatchEventToListeners'); const spyClearDataForStorageKey = sinon.stub(target.storageAgent(), 'invoke_clearDataForStorageKey'); - Resources.StorageView.StorageView.clearByStorageKey( - target, testKey, undefined, [Protocol.Storage.StorageType.All], false); + Resources.StorageView.StorageView.clear(target, testKey, null, [Protocol.Storage.StorageType.All], false); // must be called 4 times, twice with DOMStorageRemoved for local and non-local storage and twice with DOMStorageAdded assert.isTrue(spyClearDataForStorageKey.calledOnce); assert.strictEqual(dispatcherSpy.callCount, 4); @@ -61,20 +60,19 @@ describeWithMockConnection('StorageView', () => { assert.strictEqual(subtitle?.textContent, testKey); }); - it('also clears cookies on clearByStorageKey', () => { + it('also clears cookies on clear', () => { /* eslint-disable-next-line @typescript-eslint/no-non-null-assertion */ const cookieModel = target.model(SDK.CookieModel.CookieModel)!; const clearByOriginSpy = sinon.spy(target.storageAgent(), 'invoke_clearDataForOrigin'); const cookieClearSpy = sinon.spy(cookieModel, 'clear'); - Resources.StorageView.StorageView.clearByStorageKey( - target, testKey, testOrigin, [Protocol.Storage.StorageType.All], false); + Resources.StorageView.StorageView.clear(target, testKey, testOrigin, [Protocol.Storage.StorageType.All], false); assert.isTrue(clearByOriginSpy.calledOnceWithExactly({origin: testOrigin, storageTypes: 'cookies'})); assert.isTrue(cookieClearSpy.calledOnceWithExactly(undefined, testOrigin)); }); - it('also clears WebSQL on clearByStorageKey', async () => { + it('also clears WebSQL on clear', async () => { const databaseModel = target.model(Resources.DatabaseModel.DatabaseModel); assertNotNullOrUndefined(databaseModel); const databaseRemoved = new Promise(resolve => { @@ -86,8 +84,7 @@ describeWithMockConnection('StorageView', () => { databaseModel.addDatabase(testDatabase); assert.deepEqual(databaseModel.databases()[0], testDatabase); - Resources.StorageView.StorageView.clearByStorageKey( - target, testKey, '', [Protocol.Storage.StorageType.All], false); + Resources.StorageView.StorageView.clear(target, testKey, '', [Protocol.Storage.StorageType.All], false); await databaseRemoved; assert.isEmpty(databaseModel.databases()); @@ -121,7 +118,7 @@ describeWithMockConnection('StorageView', () => { assert.isEmpty(databaseModel.databases()); }); - it('clears cache on clearByStorageKey', async () => { + it('clears cache on clear', async () => { const cacheStorageModel = target.model(SDK.ServiceWorkerCacheModel.ServiceWorkerCacheModel); assertNotNullOrUndefined(cacheStorageModel); let caches = [ @@ -149,8 +146,7 @@ describeWithMockConnection('StorageView', () => { await cacheAddedPromise; caches = []; - Resources.StorageView.StorageView.clearByStorageKey( - target, testKey, '', [Protocol.Storage.StorageType.Cache_storage], false); + Resources.StorageView.StorageView.clear(target, testKey, '', [Protocol.Storage.StorageType.Cache_storage], false); assert.isEmpty(cacheStorageModel.caches()); });