diff --git a/apps/browser/src/background/main.background.ts b/apps/browser/src/background/main.background.ts index c271dd29db..4c9a51a388 100644 --- a/apps/browser/src/background/main.background.ts +++ b/apps/browser/src/background/main.background.ts @@ -1477,16 +1477,7 @@ export default class MainBackground { return; } - const storage = await this.storageService.getAll(); - await this.storageService.clear(); - - for (const key in storage) { - // eslint-disable-next-line - if (!storage.hasOwnProperty(key)) { - continue; - } - await this.storageService.save(key, storage[key]); - } + await this.storageService.reseed(); } async clearClipboard(clipboardValue: string, clearMs: number) { diff --git a/apps/browser/src/platform/services/abstractions/abstract-chrome-storage-api.service.ts b/apps/browser/src/platform/services/abstractions/abstract-chrome-storage-api.service.ts index 2bdc2fd0d4..4690562fd7 100644 --- a/apps/browser/src/platform/services/abstractions/abstract-chrome-storage-api.service.ts +++ b/apps/browser/src/platform/services/abstractions/abstract-chrome-storage-api.service.ts @@ -10,8 +10,17 @@ import { fromChromeEvent } from "../../browser/from-chrome-event"; export const serializationIndicator = "__json__"; -type serializedObject = { [serializationIndicator]: true; value: string }; +export type SerializedValue = { [serializationIndicator]: true; value: string }; +/** + * Serializes the given object and decorates it to indicate it is serialized. + * + * We have the problem that it is difficult to tell when a value has been serialized, by always + * storing objects decorated with this method, we can easily tell when a value has been serialized and + * deserialize it appropriately. + * @param obj object to decorate and serialize + * @returns a serialized version of the object, decorated to indicate that it is serialized + */ export const objToStore = (obj: any) => { if (obj == null) { return null; @@ -22,7 +31,7 @@ export const objToStore = (obj: any) => { } return { - [serializationIndicator]: true, + [serializationIndicator]: true as const, value: JSON.stringify(obj), }; }; @@ -105,7 +114,7 @@ export default abstract class AbstractChromeStorageService } /** Backwards compatible resolution of retrieved object with new serialized storage */ - protected processGetObject(obj: T | serializedObject): T | null { + protected processGetObject(obj: T | SerializedValue): T | null { if (this.isSerialized(obj)) { obj = JSON.parse(obj.value); } @@ -113,8 +122,8 @@ export default abstract class AbstractChromeStorageService } /** Type guard for whether an object is tagged as serialized */ - protected isSerialized(value: T | serializedObject): value is serializedObject { - const asSerialized = value as serializedObject; + protected isSerialized(value: T | SerializedValue): value is SerializedValue { + const asSerialized = value as SerializedValue; return ( asSerialized != null && asSerialized[serializationIndicator] && 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 index 37ea37dbf6..bd79dd6fa5 100644 --- a/apps/browser/src/platform/services/browser-local-storage.service.spec.ts +++ b/apps/browser/src/platform/services/browser-local-storage.service.spec.ts @@ -1,89 +1,173 @@ import { objToStore } from "./abstractions/abstract-chrome-storage-api.service"; -import BrowserLocalStorageService from "./browser-local-storage.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 = {}; - service = new BrowserLocalStorageService(); - }); - - describe("clear", () => { - let clearMock: jest.Mock; - - beforeEach(() => { - clearMock = chrome.storage.local.clear as jest.Mock; + // Record change listener + chrome.storage.local.onChanged.addListener = jest.fn((listener) => { + changeListener = listener; }); - it("uses the api to clear", async () => { - await service.clear(); + 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(() => { + 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); }); }); - describe("getAll", () => { - let getMock: jest.Mock; + describe.each(["get", "has", "save", "remove"] as const)("%s", (method) => { + let interval: string | number | NodeJS.Timeout; - beforeEach(() => { - // setup get - getMock = chrome.storage.local.get as jest.Mock; - getMock.mockImplementation((key, callback) => { - if (key == null) { - callback(store); - } else { - callback({ [key]: store[key] }); - } - }); + afterEach(() => { + if (interval) { + clearInterval(interval); + } }); - it("returns all values", async () => { - store["key1"] = "string"; - store["key2"] = 0; - const result = await service.getAll(); + function startReseed() { + store[RESEED_IN_PROGRESS_KEY] = objToStore(true); + } - expect(result).toEqual(store); + 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("handles empty stores", async () => { - const result = await service.getAll(); - - expect(result).toEqual({}); + it("does not wait if reseed is not in progress", async () => { + const promise = service[method]("key", "value"); + await expect(promise).toBeResolved(1); }); - it("handles stores with null values", async () => { - store["key"] = null; + it("awaits prior reseed operations before starting a new one", async () => { + startReseed(); - const result = await service.getAll(); - expect(result).toEqual(store); - }); + const promise = service.reseed(); - it("handles values processed for storage", async () => { - const obj = { test: 2 }; - const key = "key"; - store[key] = objToStore(obj); + await expect(promise).not.toBeFulfilled(10); - const result = await service.getAll(); + endReseed(); - expect(result).toEqual({ - [key]: obj, - }); - }); - - // This is a test of backwards compatibility before local storage was serialized. - it("handles values that were stored without processing for storage", async () => { - const obj = { test: 2 }; - const key = "key"; - store[key] = obj; - - const result = await service.getAll(); - - expect(result).toEqual({ - [key]: obj, - }); + 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 e1f9f63676..7957b6edea 100644 --- a/apps/browser/src/platform/services/browser-local-storage.service.ts +++ b/apps/browser/src/platform/services/browser-local-storage.service.ts @@ -1,15 +1,91 @@ -import AbstractChromeStorageService from "./abstractions/abstract-chrome-storage-api.service"; +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"; 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); + } + } + + 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 */ - async clear() { - await chrome.storage.local.clear(); + private async clear() { + return new Promise((resolve) => { + this.chromeStorageApi.clear(() => { + resolve(); + }); + }); } /** @@ -18,7 +94,7 @@ export default class BrowserLocalStorageService extends AbstractChromeStorageSer * @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 */ - async getAll(): Promise> { + private async getAll(): Promise> { return new Promise((resolve) => { this.chromeStorageApi.get(null, (allStorage) => { const resolved = Object.entries(allStorage).reduce( @@ -32,4 +108,19 @@ export default class BrowserLocalStorageService extends AbstractChromeStorageSer }); }); } + + private async saveAll(data: Record): Promise { + return new Promise((resolve) => { + const keyedData = Object.entries(data).reduce( + (agg, [key, value]) => { + agg[key] = objToStore(value); + return agg; + }, + {} as Record, + ); + this.chromeStorageApi.set(keyedData, () => { + resolve(); + }); + }); + } } diff --git a/apps/browser/test.setup.ts b/apps/browser/test.setup.ts index 2c358b62c4..7f10b4075f 100644 --- a/apps/browser/test.setup.ts +++ b/apps/browser/test.setup.ts @@ -1,4 +1,7 @@ import "jest-preset-angular/setup-jest"; +import { addCustomMatchers } from "@bitwarden/common/spec"; + +addCustomMatchers(); // Add chrome storage api const QUOTA_BYTES = 10; @@ -10,6 +13,10 @@ const storage = { QUOTA_BYTES, getBytesInUse: jest.fn(), clear: jest.fn(), + onChanged: { + addListener: jest.fn(), + removeListener: jest.fn(), + }, }, session: { set: jest.fn(), diff --git a/apps/browser/tsconfig.json b/apps/browser/tsconfig.json index beb4a73212..b44d994f4e 100644 --- a/apps/browser/tsconfig.json +++ b/apps/browser/tsconfig.json @@ -43,6 +43,7 @@ "include": [ "src", "../../libs/common/src/platform/services/**/*.worker.ts", - "../../libs/common/src/autofill/constants" + "../../libs/common/src/autofill/constants", + "../../libs/common/custom-matchers.d.ts" ] } diff --git a/libs/common/custom-matchers.d.ts b/libs/common/custom-matchers.d.ts index 214529ff02..5dd30dac79 100644 --- a/libs/common/custom-matchers.d.ts +++ b/libs/common/custom-matchers.d.ts @@ -1,4 +1,4 @@ -import type { CustomMatchers } from "./test.setup"; +import type { CustomMatchers } from "./spec"; // This declares the types for our custom matchers so that they're recognised by Typescript // This file must also be included in the TS compilation (via the tsconfig.json "include" property) to be recognised by diff --git a/libs/common/spec/matchers/index.ts b/libs/common/spec/matchers/index.ts index 59f6409fef..235f54d775 100644 --- a/libs/common/spec/matchers/index.ts +++ b/libs/common/spec/matchers/index.ts @@ -1 +1,57 @@ +import { toBeFulfilled, toBeResolved, toBeRejected } from "./promise-fulfilled"; +import { toAlmostEqual } from "./to-almost-equal"; +import { toEqualBuffer } from "./to-equal-buffer"; + export * from "./to-equal-buffer"; +export * from "./to-almost-equal"; +export * from "./promise-fulfilled"; + +export function addCustomMatchers() { + expect.extend({ + toEqualBuffer: toEqualBuffer, + toAlmostEqual: toAlmostEqual, + toBeFulfilled: toBeFulfilled, + toBeResolved: toBeResolved, + toBeRejected: toBeRejected, + }); +} + +export interface CustomMatchers { + toEqualBuffer(expected: Uint8Array | ArrayBuffer): R; + /** + * Matches the expected date within an optional ms precision + * @param expected The expected date + * @param msPrecision The optional precision in milliseconds + */ + toAlmostEqual(expected: Date, msPrecision?: number): R; + /** + * Matches whether the received promise has been fulfilled. + * + * Failure if the promise is not currently fulfilled. + * + * @param received The promise to test + * @param withinMs The time within the promise should be fulfilled. Defaults to 0, indicating that the promise should already be fulfilled + * @returns CustomMatcherResult indicating whether or not the test passed + */ + toBeFulfilled(withinMs?: number): Promise; + /** + * Matches whether the received promise has been resolved. + * + * Failure if the promise is not currently fulfilled or if it has been rejected. + * + * @param received The promise to test + * @param withinMs The time within the promise should be resolved. Defaults to 0, indicating that the promise should already be resolved + * @returns CustomMatcherResult indicating whether or not the test passed + */ + toBeResolved(withinMs?: number): Promise; + /** + * Matches whether the received promise has been rejected. + * + * Failure if the promise is not currently fulfilled or if it has been resolved, but not rejected. + * + * @param received The promise to test + * @param withinMs The time within the promise should be rejected. Defaults to 0, indicating that the promise should already be rejected + * @returns CustomMatcherResult indicating whether or not the test passed + */ + toBeRejected(withinMs?: number): Promise; +} diff --git a/libs/common/spec/matchers/promise-fulfilled.spec.ts b/libs/common/spec/matchers/promise-fulfilled.spec.ts new file mode 100644 index 0000000000..80c7b0122f --- /dev/null +++ b/libs/common/spec/matchers/promise-fulfilled.spec.ts @@ -0,0 +1,86 @@ +describe("toBeFulfilled", () => { + it("passes when promise is resolved", async () => { + const promise = Promise.resolve("resolved"); + await promise; + await expect(promise).toBeFulfilled(); + }); + + it("passes when promise is rejected", async () => { + const promise = Promise.reject("rejected"); + await promise.catch(() => {}); + await expect(promise).toBeFulfilled(); + }); + + it("fails when promise is pending", async () => { + const promise = new Promise((resolve) => setTimeout(resolve, 1000)); + await expect(promise).not.toBeFulfilled(); + }); + + it("passes when the promise is fulfilled within the given time limit", async () => { + const promise = new Promise((resolve) => setTimeout(resolve, 1)); + await expect(promise).toBeFulfilled(2); + }); + + it("passes when the promise is not fulfilled within the given time limit", async () => { + const promise = new Promise((resolve) => setTimeout(resolve, 2)); + await expect(promise).not.toBeFulfilled(1); + }); +}); + +describe("toBeResolved", () => { + it("passes when promise is resolved", async () => { + const promise = Promise.resolve("resolved"); + await promise; + await expect(promise).toBeResolved(); + }); + + it("fails when promise is rejected", async () => { + const promise = Promise.reject("rejected"); + await promise.catch(() => {}); + await expect(promise).not.toBeResolved(); + }); + + it("fails when promise is pending", async () => { + const promise = new Promise((resolve) => setTimeout(resolve, 1000)); + await expect(promise).not.toBeResolved(); + }); + + it("passes when the promise is resolved within the given time limit", async () => { + const promise = new Promise((resolve) => setTimeout(resolve, 1)); + await expect(promise).toBeResolved(2); + }); + + it("passes when the promise is not resolved within the given time limit", async () => { + const promise = new Promise((resolve) => setTimeout(resolve, 2)); + await expect(promise).not.toBeResolved(1); + }); +}); + +describe("toBeRejected", () => { + it("fails when promise is resolved", async () => { + const promise = Promise.resolve("resolved"); + await promise; + await expect(promise).not.toBeRejected(); + }); + + it("passes when promise is rejected", async () => { + const promise = Promise.reject("rejected"); + await promise.catch(() => {}); + await expect(promise).toBeRejected(); + }); + + it("fails when promise is pending", async () => { + const promise = new Promise((resolve) => setTimeout(resolve, 1000)); + await expect(promise).not.toBeRejected(); + }); + + it("passes when the promise is resolved within the given time limit", async () => { + const promise = new Promise((_, reject) => setTimeout(reject, 1)); + await expect(promise).toBeFulfilled(2); + }); + + it("passes when the promise is not resolved within the given time limit", async () => { + const promise = new Promise((_, reject) => setTimeout(reject, 2)); + await expect(promise).not.toBeFulfilled(1); + }); +}); diff --git a/libs/common/spec/matchers/promise-fulfilled.ts b/libs/common/spec/matchers/promise-fulfilled.ts new file mode 100644 index 0000000000..c29c608a2c --- /dev/null +++ b/libs/common/spec/matchers/promise-fulfilled.ts @@ -0,0 +1,79 @@ +async function wait(ms: number): Promise { + return new Promise((resolve) => setTimeout(resolve, ms)); +} + +/** + * Matches whether the received promise has been fulfilled. + * + * Failure if the promise is not currently fulfilled. + * + * @param received The promise to test + * @param withinMs The time within the promise should be fulfilled. Defaults to 0, indicating that the promise should already be fulfilled + * @returns CustomMatcherResult indicating whether or not the test passed + */ +export const toBeFulfilled: jest.CustomMatcher = async function ( + received: Promise, + withinMs = 0, +) { + return { + pass: await Promise.race([ + wait(withinMs).then(() => false), + received.then( + () => true, + () => true, + ), + ]), + message: () => `expected promise to be fulfilled`, + }; +}; + +/** + * Matches whether the received promise has been resolved. + * + * Failure if the promise is not currently fulfilled or if it has been rejected. + * + * @param received The promise to test + * @param withinMs The time within the promise should be resolved. Defaults to 0, indicating that the promise should already be resolved + * @returns CustomMatcherResult indicating whether or not the test passed + + */ +export const toBeResolved: jest.CustomMatcher = async function ( + received: Promise, + withinMs = 0, +) { + return { + pass: await Promise.race([ + wait(withinMs).then(() => false), + received.then( + () => true, + () => false, + ), + ]), + message: () => `expected promise to be resolved`, + }; +}; + +/** + * Matches whether the received promise has been rejected. + * + * Failure if the promise is not currently fulfilled or if it has been resolved, but not rejected. + * + * @param received The promise to test + * @param withinMs The time within the promise should be rejected. Defaults to 0, indicating that the promise should already be rejected + * @returns CustomMatcherResult indicating whether or not the test passed + */ +export const toBeRejected: jest.CustomMatcher = async function ( + received: Promise, + withinMs = 0, +) { + return { + pass: await Promise.race([ + wait(withinMs).then(() => false), + received.then( + () => false, + () => true, + ), + ]), + message: () => `expected promise to be rejected`, + }; +}; diff --git a/libs/common/test.setup.ts b/libs/common/test.setup.ts index d857751b51..aa71b3e508 100644 --- a/libs/common/test.setup.ts +++ b/libs/common/test.setup.ts @@ -1,25 +1,10 @@ import { webcrypto } from "crypto"; -import { toEqualBuffer } from "./spec"; -import { toAlmostEqual } from "./spec/matchers/to-almost-equal"; +import { addCustomMatchers } from "./spec"; Object.defineProperty(window, "crypto", { value: webcrypto, }); // Add custom matchers - -expect.extend({ - toEqualBuffer: toEqualBuffer, - toAlmostEqual: toAlmostEqual, -}); - -export interface CustomMatchers { - toEqualBuffer(expected: Uint8Array | ArrayBuffer): R; - /** - * Matches the expected date within an optional ms precision - * @param expected The expected date - * @param msPrecision The optional precision in milliseconds - */ - toAlmostEqual(expected: Date, msPrecision?: number): R; -} +addCustomMatchers();