[PM-8847] Delay browser local storage operations during reseed (#9536)

* Define matchers to test promise fulfillment

These are useful for validating that promises depend on other events prior to fulfilling.

* Expose custom matchers to jest projects

Team-specific projects are not touched here to try and reduce review burden.

* Block browser local operations awaiting reseed

This should closes a narrow race condition resulting from storage operations during a reseed event.

* Import from barrel file

This might fix the failing test, but I'm not sure _why_

* Document helper methods

* Validate as few properties as possible per test

* Simplify expected value representation

* Allow waiting in promise matchers

* Specify resolution times in promise orchestration tests.

* Test behavior triggering multiple reseeds.

* Fix typo

* Avoid testing implementation details

* Clear reseed on startup

in case a previous process was aborted in the middle of a reseed.

* Correct formatting

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) {

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<T>(obj: T | serializedObject): T | null {
+  protected processGetObject<T>(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<T>(value: T | serializedObject): value is serializedObject {
-    const asSerialized = value as serializedObject;
+  protected isSerialized<T>(value: T | SerializedValue): value is SerializedValue {
+    const asSerialized = value as SerializedValue;
     return (
       asSerialized != null &&
       asSerialized[serializationIndicator] &&

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<any, any>) => (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<any, any>;
+  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<keyof typeof testStore>) {
+        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<keyof typeof testStore>) {
+        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();
     });
   });
 });

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<void> {
+    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<T>(key: string): Promise<T> {
+    await this.awaitReseed();
+    return super.get(key);
+  }
+
+  override async has(key: string): Promise<boolean> {
+    await this.awaitReseed();
+    return super.has(key);
+  }
+
+  override async save(key: string, obj: any): Promise<void> {
+    await this.awaitReseed();
+    return super.save(key, obj);
+  }
+
+  override async remove(key: string): Promise<void> {
+    await this.awaitReseed();
+    return super.remove(key);
+  }
+
+  private async awaitReseed(): Promise<void> {
+    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<void>((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<Record<string, unknown>> {
+  private async getAll(): Promise<Record<string, unknown>> {
     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<string, unknown>): Promise<void> {
+    return new Promise<void>((resolve) => {
+      const keyedData = Object.entries(data).reduce(
+        (agg, [key, value]) => {
+          agg[key] = objToStore(value);
+          return agg;
+        },
+        {} as Record<string, SerializedValue>,
+      );
+      this.chromeStorageApi.set(keyedData, () => {
+        resolve();
+      });
+    });
+  }
 }

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(),

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"
   ]
 }

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

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<R = unknown> {
+  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<R>;
+  /**
+   * 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<R>;
+  /**
+   * 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<R>;
+}

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);
+  });
+});

libs/common/spec/matchers/promise-fulfilled.ts

@@ -0,0 +1,79 @@
+async function wait(ms: number): Promise<void> {
+  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<unknown>,
+  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<unknown>,
+  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<unknown>,
+  withinMs = 0,
+) {
+  return {
+    pass: await Promise.race([
+      wait(withinMs).then(() => false),
+      received.then(
+        () => false,
+        () => true,
+      ),
+    ]),
+    message: () => `expected promise to be rejected`,
+  };
+};

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<R = unknown> {
-  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();