From 972339be837e4757bbca92d0202673eba09d671d Mon Sep 17 00:00:00 2001 From: Justin Baur <19896123+justindbaur@users.noreply.github.com> Date: Fri, 20 Sep 2024 11:46:00 -0400 Subject: [PATCH] Remove Storage Reseed FF (#11156) --- .../browser/src/background/main.background.ts | 17 +- .../src/background/runtime.background.ts | 19 +- .../browser-local-storage.service.spec.ts | 192 ------------------ .../services/browser-local-storage.service.ts | 132 +----------- libs/common/src/enums/feature-flag.enum.ts | 2 - 5 files changed, 6 insertions(+), 356 deletions(-) delete mode 100644 apps/browser/src/platform/services/browser-local-storage.service.spec.ts diff --git a/apps/browser/src/background/main.background.ts b/apps/browser/src/background/main.background.ts index 62d83b1900..18883a5fe5 100644 --- a/apps/browser/src/background/main.background.ts +++ b/apps/browser/src/background/main.background.ts @@ -1484,14 +1484,7 @@ export default class MainBackground { }); if (needStorageReseed) { - await this.reseedStorage( - await firstValueFrom( - this.configService.userCachedFeatureFlag$( - FeatureFlag.StorageReseedRefactor, - userBeingLoggedOut, - ), - ), - ); + await this.reseedStorage(); } if (BrowserApi.isManifestVersion(3)) { @@ -1546,7 +1539,7 @@ export default class MainBackground { await SafariApp.sendMessageToApp("showPopover", null, true); } - async reseedStorage(doFillBuffer: boolean) { + async reseedStorage() { if ( !this.platformUtilsService.isChrome() && !this.platformUtilsService.isVivaldi() && @@ -1555,11 +1548,7 @@ export default class MainBackground { return; } - if (doFillBuffer) { - await this.storageService.fillBuffer(); - } else { - await this.storageService.reseed(); - } + await this.storageService.fillBuffer(); } async clearClipboard(clipboardValue: string, clearMs: number) { diff --git a/apps/browser/src/background/runtime.background.ts b/apps/browser/src/background/runtime.background.ts index 1ec7edcc30..3e0933942b 100644 --- a/apps/browser/src/background/runtime.background.ts +++ b/apps/browser/src/background/runtime.background.ts @@ -1,4 +1,4 @@ -import { firstValueFrom, map, mergeMap, of, switchMap } from "rxjs"; +import { firstValueFrom, map, mergeMap } from "rxjs"; import { LockService } from "@bitwarden/auth/common"; import { NotificationsService } from "@bitwarden/common/abstractions/notifications.service"; @@ -281,22 +281,7 @@ export default class RuntimeBackground { await this.main.refreshMenu(); break; case "bgReseedStorage": { - const doFillBuffer = await firstValueFrom( - this.accountService.activeAccount$.pipe( - switchMap((account) => { - if (account == null) { - return of(false); - } - - return this.configService.userCachedFeatureFlag$( - FeatureFlag.StorageReseedRefactor, - account.id, - ); - }), - ), - ); - - await this.main.reseedStorage(doFillBuffer); + await this.main.reseedStorage(); break; } case "authResult": { diff --git a/apps/browser/src/platform/services/browser-local-storage.service.spec.ts b/apps/browser/src/platform/services/browser-local-storage.service.spec.ts deleted file mode 100644 index 13e26c26dd..0000000000 --- a/apps/browser/src/platform/services/browser-local-storage.service.spec.ts +++ /dev/null @@ -1,192 +0,0 @@ -import { objToStore } from "./abstractions/abstract-chrome-storage-api.service"; -import BrowserLocalStorageService, { - RESEED_IN_PROGRESS_KEY, -} from "./browser-local-storage.service"; - -const apiGetLike = - (store: Record) => (key: string, callback: (items: { [key: string]: any }) => void) => { - if (key == null) { - callback(store); - } else { - callback({ [key]: store[key] }); - } - }; - -describe("BrowserLocalStorageService", () => { - let service: BrowserLocalStorageService; - let store: Record; - let changeListener: (changes: { [key: string]: chrome.storage.StorageChange }) => void; - - let saveMock: jest.Mock; - let getMock: jest.Mock; - let clearMock: jest.Mock; - let removeMock: jest.Mock; - - beforeEach(() => { - store = {}; - - // Record change listener - chrome.storage.local.onChanged.addListener = jest.fn((listener) => { - changeListener = listener; - }); - - service = new BrowserLocalStorageService(); - - // setup mocks - getMock = chrome.storage.local.get as jest.Mock; - getMock.mockImplementation(apiGetLike(store)); - saveMock = chrome.storage.local.set as jest.Mock; - saveMock.mockImplementation((update, callback) => { - Object.entries(update).forEach(([key, value]) => { - store[key] = value; - }); - callback(); - }); - clearMock = chrome.storage.local.clear as jest.Mock; - clearMock.mockImplementation((callback) => { - store = {}; - callback?.(); - }); - removeMock = chrome.storage.local.remove as jest.Mock; - removeMock.mockImplementation((keys, callback) => { - if (Array.isArray(keys)) { - keys.forEach((key) => { - delete store[key]; - }); - } else { - delete store[keys]; - } - - callback(); - }); - }); - - afterEach(() => { - chrome.runtime.lastError = undefined; - jest.resetAllMocks(); - }); - - describe("reseed", () => { - it.each([ - { - key1: objToStore("value1"), - key2: objToStore("value2"), - key3: null, - }, - {}, - ])("saves all data in storage %s", async (testStore) => { - for (const key of Object.keys(testStore) as Array) { - store[key] = testStore[key]; - } - await service.reseed(); - - expect(saveMock).toHaveBeenLastCalledWith( - { ...testStore, [RESEED_IN_PROGRESS_KEY]: objToStore(true) }, - expect.any(Function), - ); - }); - - it.each([ - { - key1: objToStore("value1"), - key2: objToStore("value2"), - key3: null, - }, - {}, - ])("results in the same store %s", async (testStore) => { - for (const key of Object.keys(testStore) as Array) { - store[key] = testStore[key]; - } - await service.reseed(); - - expect(store).toEqual(testStore); - }); - - it("converts non-serialized values to serialized", async () => { - store.key1 = "value1"; - store.key2 = "value2"; - - const expectedStore = { - key1: objToStore("value1"), - key2: objToStore("value2"), - reseedInProgress: objToStore(true), - }; - - await service.reseed(); - - expect(saveMock).toHaveBeenLastCalledWith(expectedStore, expect.any(Function)); - }); - - it("clears data", async () => { - await service.reseed(); - - expect(clearMock).toHaveBeenCalledTimes(1); - }); - - it("throws if get has chrome.runtime.lastError", async () => { - getMock.mockImplementation((key, callback) => { - chrome.runtime.lastError = new Error("Get Test Error"); - callback(); - }); - - await expect(async () => await service.reseed()).rejects.toThrow("Get Test Error"); - }); - - it("throws if save has chrome.runtime.lastError", async () => { - saveMock.mockImplementation((obj, callback) => { - chrome.runtime.lastError = new Error("Save Test Error"); - callback(); - }); - - await expect(async () => await service.reseed()).rejects.toThrow("Save Test Error"); - }); - }); - - describe.each(["get", "has", "save", "remove"] as const)("%s", (method) => { - let interval: string | number | NodeJS.Timeout; - - afterEach(() => { - if (interval) { - clearInterval(interval); - } - }); - - function startReseed() { - store[RESEED_IN_PROGRESS_KEY] = objToStore(true); - } - - function endReseed() { - delete store[RESEED_IN_PROGRESS_KEY]; - changeListener({ reseedInProgress: { oldValue: true } }); - } - - it("waits for reseed prior to operation", async () => { - startReseed(); - - const promise = service[method]("key", "value"); // note "value" is only used in save, but ignored in other methods - - await expect(promise).not.toBeFulfilled(10); - - endReseed(); - - await expect(promise).toBeResolved(); - }); - - it("does not wait if reseed is not in progress", async () => { - const promise = service[method]("key", "value"); - await expect(promise).toBeResolved(1); - }); - - it("awaits prior reseed operations before starting a new one", async () => { - startReseed(); - - const promise = service.reseed(); - - await expect(promise).not.toBeFulfilled(10); - - endReseed(); - - await expect(promise).toBeResolved(); - }); - }); -}); diff --git a/apps/browser/src/platform/services/browser-local-storage.service.ts b/apps/browser/src/platform/services/browser-local-storage.service.ts index 15cf26d1fb..61a2653f13 100644 --- a/apps/browser/src/platform/services/browser-local-storage.service.ts +++ b/apps/browser/src/platform/services/browser-local-storage.service.ts @@ -1,35 +1,8 @@ -import { defer, filter, firstValueFrom, map, merge, throwError, timeout } from "rxjs"; - -import AbstractChromeStorageService, { - SerializedValue, - objToStore, -} from "./abstractions/abstract-chrome-storage-api.service"; - -export const RESEED_IN_PROGRESS_KEY = "reseedInProgress"; +import AbstractChromeStorageService from "./abstractions/abstract-chrome-storage-api.service"; export default class BrowserLocalStorageService extends AbstractChromeStorageService { constructor() { super(chrome.storage.local); - this.chromeStorageApi.remove(RESEED_IN_PROGRESS_KEY, () => { - return; - }); - } - - /** - * Reads, clears, and re-saves all data in local storage. This is a hack to remove previously stored sensitive data from - * local storage logs. - * - * @see https://github.com/bitwarden/clients/issues/485 - */ - async reseed(): Promise { - try { - await this.save(RESEED_IN_PROGRESS_KEY, true); - const data = await this.getAll(); - await this.clear(); - await this.saveAll(data); - } finally { - await super.remove(RESEED_IN_PROGRESS_KEY); - } } async fillBuffer() { @@ -71,107 +44,4 @@ export default class BrowserLocalStorageService extends AbstractChromeStorageSer ); }); } - - override async get(key: string): Promise { - await this.awaitReseed(); - return super.get(key); - } - - override async has(key: string): Promise { - await this.awaitReseed(); - return super.has(key); - } - - override async save(key: string, obj: any): Promise { - await this.awaitReseed(); - return super.save(key, obj); - } - - override async remove(key: string): Promise { - await this.awaitReseed(); - return super.remove(key); - } - - private async awaitReseed(): Promise { - const notReseeding = async () => { - return !(await super.get(RESEED_IN_PROGRESS_KEY)); - }; - - const finishedReseeding = this.updates$.pipe( - filter(({ key, updateType }) => key === RESEED_IN_PROGRESS_KEY && updateType === "remove"), - map(() => true), - ); - - await firstValueFrom( - merge(defer(notReseeding), finishedReseeding).pipe( - filter((v) => v), - timeout({ - // We eventually need to give up and throw an error - first: 5_000, - with: () => - throwError( - () => new Error("Reseeding local storage did not complete in a timely manner."), - ), - }), - ), - ); - } - - /** - * Clears local storage - */ - private async clear() { - return new Promise((resolve, reject) => { - this.chromeStorageApi.clear(() => { - if (chrome.runtime.lastError) { - return reject(chrome.runtime.lastError); - } - resolve(); - }); - }); - } - - /** - * Retrieves all objects stored in local storage. - * - * @remarks This method processes values prior to resolving, do not use `chrome.storage.local` directly - * @returns Promise resolving to keyed object of all stored data - */ - private async getAll(): Promise> { - return new Promise((resolve, reject) => { - this.chromeStorageApi.get(null, (allStorage) => { - if (chrome.runtime.lastError) { - return reject(chrome.runtime.lastError); - } - - const resolved = Object.entries(allStorage).reduce( - (agg, [key, value]) => { - agg[key] = this.processGetObject(value); - return agg; - }, - {} as Record, - ); - resolve(resolved); - }); - }); - } - - private async saveAll(data: Record): Promise { - return new Promise((resolve, reject) => { - const keyedData = Object.entries(data).reduce( - (agg, [key, value]) => { - agg[key] = objToStore(value); - return agg; - }, - {} as Record, - ); - this.chromeStorageApi.set(keyedData, () => { - if (chrome.runtime.lastError) { - return reject(chrome.runtime.lastError); - } - - resolve(); - }); - }); - } } diff --git a/libs/common/src/enums/feature-flag.enum.ts b/libs/common/src/enums/feature-flag.enum.ts index e7ece5b36a..3d426aee3b 100644 --- a/libs/common/src/enums/feature-flag.enum.ts +++ b/libs/common/src/enums/feature-flag.enum.ts @@ -34,7 +34,6 @@ export enum FeatureFlag { AccountDeprovisioning = "pm-10308-account-deprovisioning", NotificationBarAddLoginImprovements = "notification-bar-add-login-improvements", AC2476_DeprecateStripeSourcesAPI = "AC-2476-deprecate-stripe-sources-api", - StorageReseedRefactor = "storage-reseed-refactor", CipherKeyEncryption = "cipher-key-encryption", } @@ -77,7 +76,6 @@ export const DefaultFeatureFlagValue = { [FeatureFlag.GenerateIdentityFillScriptRefactor]: FALSE, [FeatureFlag.EnableNewCardCombinedExpiryAutofill]: FALSE, [FeatureFlag.DelayFido2PageScriptInitWithinMv2]: FALSE, - [FeatureFlag.StorageReseedRefactor]: FALSE, [FeatureFlag.AccountDeprovisioning]: FALSE, [FeatureFlag.NotificationBarAddLoginImprovements]: FALSE, [FeatureFlag.AC2476_DeprecateStripeSourcesAPI]: FALSE,