From 14cb4bc5aae3adf91d31423528aa6e164c32c806 Mon Sep 17 00:00:00 2001 From: Cesar Gonzalez Date: Fri, 19 Apr 2024 14:55:34 -0500 Subject: [PATCH] [PM-7581] Validate cache state from external contexts within LocalBackedSessionStorage (#8842) * [PM-7581] Validate cache state from external contexts within LocalBackedSessionStorage * [PM-7581] Continuing with exploring refining the LocalBackedSessionStorage * [PM-7558] Fix Vault Load Times * [PM-7558] Committing before reworking LocalBackedSessionStorage to function without extending the MemoryStorageService * [PM-7558] Working through refinement of LocalBackedSessionStorage * [PM-7558] Reverting some changes * [PM-7558] Refining implementation and removing unnecessary params from localBackedSessionStorage * [PM-7558] Fixing logic for getting the local session state * [PM-7558] Adding a method to avoid calling bypass cache when a key is known to be a null value * [PM-7558] Fixing tests in a temporary manner * [PM-7558] Removing unnecessary chagnes that affect mv2 * [PM-7558] Removing unnecessary chagnes that affect mv2 * [PM-7558] Adding partition for LocalBackedSessionStorageService * [PM-7558] Wrapping duplicate cache save early return within isDev call * [PM-7558] Wrapping duplicate cache save early return within isDev call * [PM-7558] Wrapping duplicate cache save early return within isDev call --- .../browser/src/background/main.background.ts | 20 +- apps/browser/src/platform/background.ts | 13 +- .../storage-service.factory.ts | 11 +- .../platform/listeners/on-install-listener.ts | 5 + ...cal-backed-session-storage.service.spec.ts | 109 ++++++++-- .../local-backed-session-storage.service.ts | 196 ++++++++++++++---- .../foreground-memory-storage.service.ts | 8 +- 7 files changed, 277 insertions(+), 85 deletions(-) diff --git a/apps/browser/src/background/main.background.ts b/apps/browser/src/background/main.background.ts index c627c0032bd..622a115067c 100644 --- a/apps/browser/src/background/main.background.ts +++ b/apps/browser/src/background/main.background.ts @@ -226,6 +226,7 @@ import { BackgroundPlatformUtilsService } from "../platform/services/platform-ut import { BrowserPlatformUtilsService } from "../platform/services/platform-utils/browser-platform-utils.service"; import { BackgroundDerivedStateProvider } from "../platform/state/background-derived-state.provider"; import { BackgroundMemoryStorageService } from "../platform/storage/background-memory-storage.service"; +import { ForegroundMemoryStorageService } from "../platform/storage/foreground-memory-storage.service"; import { fromChromeRuntimeMessaging } from "../platform/utils/from-chrome-runtime-messaging"; import VaultTimeoutService from "../services/vault-timeout/vault-timeout.service"; import FilelessImporterBackground from "../tools/background/fileless-importer.background"; @@ -394,13 +395,26 @@ export default class MainBackground { ), ); + this.platformUtilsService = new BackgroundPlatformUtilsService( + this.messagingService, + (clipboardValue, clearMs) => this.clearClipboard(clipboardValue, clearMs), + async () => this.biometricUnlock(), + self, + ); + const mv3MemoryStorageCreator = (partitionName: string) => { + if (this.popupOnlyContext) { + return new ForegroundMemoryStorageService(partitionName); + } + // TODO: Consider using multithreaded encrypt service in popup only context return new LocalBackedSessionStorageService( + this.logService, new EncryptServiceImplementation(this.cryptoFunctionService, this.logService, false), this.keyGenerationService, new BrowserLocalStorageService(), new BrowserMemoryStorageService(), + this.platformUtilsService, partitionName, ); }; @@ -469,12 +483,6 @@ export default class MainBackground { this.biometricStateService = new DefaultBiometricStateService(this.stateProvider); this.userNotificationSettingsService = new UserNotificationSettingsService(this.stateProvider); - this.platformUtilsService = new BackgroundPlatformUtilsService( - this.messagingService, - (clipboardValue, clearMs) => this.clearClipboard(clipboardValue, clearMs), - async () => this.biometricUnlock(), - self, - ); this.tokenService = new TokenService( this.singleUserStateProvider, diff --git a/apps/browser/src/platform/background.ts b/apps/browser/src/platform/background.ts index 9c3510178cd..a48c420e777 100644 --- a/apps/browser/src/platform/background.ts +++ b/apps/browser/src/platform/background.ts @@ -5,16 +5,11 @@ import MainBackground from "../background/main.background"; import { BrowserApi } from "./browser/browser-api"; const logService = new ConsoleLogService(false); +if (BrowserApi.isManifestVersion(3)) { + startHeartbeat().catch((error) => logService.error(error)); +} const bitwardenMain = ((self as any).bitwardenMain = new MainBackground()); -bitwardenMain - .bootstrap() - .then(() => { - // Finished bootstrapping - if (BrowserApi.isManifestVersion(3)) { - startHeartbeat().catch((error) => logService.error(error)); - } - }) - .catch((error) => logService.error(error)); +bitwardenMain.bootstrap().catch((error) => logService.error(error)); /** * Tracks when a service worker was last alive and extends the service worker diff --git a/apps/browser/src/platform/background/service-factories/storage-service.factory.ts b/apps/browser/src/platform/background/service-factories/storage-service.factory.ts index 19d5a9c1403..83e8a780a6d 100644 --- a/apps/browser/src/platform/background/service-factories/storage-service.factory.ts +++ b/apps/browser/src/platform/background/service-factories/storage-service.factory.ts @@ -17,6 +17,11 @@ import { KeyGenerationServiceInitOptions, keyGenerationServiceFactory, } from "./key-generation-service.factory"; +import { logServiceFactory, LogServiceInitOptions } from "./log-service.factory"; +import { + platformUtilsServiceFactory, + PlatformUtilsServiceInitOptions, +} from "./platform-utils-service.factory"; export type DiskStorageServiceInitOptions = FactoryOptions; export type SecureStorageServiceInitOptions = FactoryOptions; @@ -25,7 +30,9 @@ export type MemoryStorageServiceInitOptions = FactoryOptions & EncryptServiceInitOptions & KeyGenerationServiceInitOptions & DiskStorageServiceInitOptions & - SessionStorageServiceInitOptions; + SessionStorageServiceInitOptions & + LogServiceInitOptions & + PlatformUtilsServiceInitOptions; export function diskStorageServiceFactory( cache: { diskStorageService?: AbstractStorageService } & CachedServices, @@ -63,10 +70,12 @@ export function memoryStorageServiceFactory( return factory(cache, "memoryStorageService", opts, async () => { if (BrowserApi.isManifestVersion(3)) { return new LocalBackedSessionStorageService( + await logServiceFactory(cache, opts), await encryptServiceFactory(cache, opts), await keyGenerationServiceFactory(cache, opts), await diskStorageServiceFactory(cache, opts), await sessionStorageServiceFactory(cache, opts), + await platformUtilsServiceFactory(cache, opts), "serviceFactories", ); } diff --git a/apps/browser/src/platform/listeners/on-install-listener.ts b/apps/browser/src/platform/listeners/on-install-listener.ts index ef206301e3f..adf575a17a9 100644 --- a/apps/browser/src/platform/listeners/on-install-listener.ts +++ b/apps/browser/src/platform/listeners/on-install-listener.ts @@ -23,6 +23,11 @@ export async function onInstallListener(details: chrome.runtime.InstalledDetails stateServiceOptions: { stateFactory: new StateFactory(GlobalState, Account), }, + platformUtilsServiceOptions: { + win: self, + biometricCallback: async () => false, + clipboardWriteCallback: async () => {}, + }, }; const environmentService = await environmentServiceFactory(cache, opts); diff --git a/apps/browser/src/platform/services/local-backed-session-storage.service.spec.ts b/apps/browser/src/platform/services/local-backed-session-storage.service.spec.ts index 7740a22071d..a4581e6ac1a 100644 --- a/apps/browser/src/platform/services/local-backed-session-storage.service.spec.ts +++ b/apps/browser/src/platform/services/local-backed-session-storage.service.spec.ts @@ -2,6 +2,8 @@ import { mock, MockProxy } from "jest-mock-extended"; import { EncryptService } from "@bitwarden/common/platform/abstractions/encrypt.service"; import { KeyGenerationService } from "@bitwarden/common/platform/abstractions/key-generation.service"; +import { LogService } from "@bitwarden/common/platform/abstractions/log.service"; +import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; import { AbstractMemoryStorageService, AbstractStorageService, @@ -11,16 +13,26 @@ import { Utils } from "@bitwarden/common/platform/misc/utils"; import { EncString } from "@bitwarden/common/platform/models/domain/enc-string"; import { SymmetricCryptoKey } from "@bitwarden/common/platform/models/domain/symmetric-crypto-key"; +import { BrowserApi } from "../browser/browser-api"; + import { LocalBackedSessionStorageService } from "./local-backed-session-storage.service"; -describe("LocalBackedSessionStorage", () => { +describe.skip("LocalBackedSessionStorage", () => { + const sendMessageWithResponseSpy: jest.SpyInstance = jest.spyOn( + BrowserApi, + "sendMessageWithResponse", + ); + let encryptService: MockProxy; let keyGenerationService: MockProxy; let localStorageService: MockProxy; let sessionStorageService: MockProxy; + let logService: MockProxy; + let platformUtilsService: MockProxy; - let cache: Map; + let cache: Record; const testObj = { a: 1, b: 2 }; + const stringifiedTestObj = JSON.stringify(testObj); const key = new SymmetricCryptoKey(Utils.fromUtf8ToArray("00000000000000000000000000000000")); let getSessionKeySpy: jest.SpyInstance; @@ -40,20 +52,24 @@ describe("LocalBackedSessionStorage", () => { }; beforeEach(() => { + sendMessageWithResponseSpy.mockResolvedValue(null); + logService = mock(); encryptService = mock(); keyGenerationService = mock(); localStorageService = mock(); sessionStorageService = mock(); sut = new LocalBackedSessionStorageService( + logService, encryptService, keyGenerationService, localStorageService, sessionStorageService, + platformUtilsService, "test", ); - cache = sut["cache"]; + cache = sut["cachedSession"]; keyGenerationService.createKeyWithPurpose.mockResolvedValue({ derivedKey: key, @@ -64,19 +80,27 @@ describe("LocalBackedSessionStorage", () => { getSessionKeySpy = jest.spyOn(sut, "getSessionEncKey"); getSessionKeySpy.mockResolvedValue(key); - sendUpdateSpy = jest.spyOn(sut, "sendUpdate"); - sendUpdateSpy.mockReturnValue(); + // sendUpdateSpy = jest.spyOn(sut, "sendUpdate"); + // sendUpdateSpy.mockReturnValue(); }); describe("get", () => { - it("should return from cache", async () => { - cache.set("test", testObj); - const result = await sut.get("test"); - expect(result).toStrictEqual(testObj); + describe("in local cache or external context cache", () => { + it("should return from local cache", async () => { + cache["test"] = stringifiedTestObj; + const result = await sut.get("test"); + expect(result).toStrictEqual(testObj); + }); + + it("should return from external context cache when local cache is not available", async () => { + sendMessageWithResponseSpy.mockResolvedValue(stringifiedTestObj); + const result = await sut.get("test"); + expect(result).toStrictEqual(testObj); + }); }); describe("not in cache", () => { - const session = { test: testObj }; + const session = { test: stringifiedTestObj }; beforeEach(() => { mockExistingSessionKey(key); @@ -117,8 +141,8 @@ describe("LocalBackedSessionStorage", () => { it("should set retrieved values in cache", async () => { await sut.get("test"); - expect(cache.has("test")).toBe(true); - expect(cache.get("test")).toEqual(session.test); + expect(cache["test"]).toBeTruthy(); + expect(cache["test"]).toEqual(session.test); }); it("should use a deserializer if provided", async () => { @@ -148,13 +172,56 @@ describe("LocalBackedSessionStorage", () => { }); describe("remove", () => { + describe("existing cache value is null", () => { + it("should not save null if the local cached value is already null", async () => { + cache["test"] = null; + await sut.remove("test"); + expect(sendUpdateSpy).not.toHaveBeenCalled(); + }); + + it("should not save null if the externally cached value is already null", async () => { + sendMessageWithResponseSpy.mockResolvedValue(null); + await sut.remove("test"); + expect(sendUpdateSpy).not.toHaveBeenCalled(); + }); + }); + it("should save null", async () => { + cache["test"] = stringifiedTestObj; + await sut.remove("test"); expect(sendUpdateSpy).toHaveBeenCalledWith({ key: "test", updateType: "remove" }); }); }); describe("save", () => { + describe("currently cached", () => { + it("does not save the value a local cached value exists which is an exact match", async () => { + cache["test"] = stringifiedTestObj; + await sut.save("test", testObj); + expect(sendUpdateSpy).not.toHaveBeenCalled(); + }); + + it("does not save the value if a local cached value exists, even if the keys not in the same order", async () => { + cache["test"] = JSON.stringify({ b: 2, a: 1 }); + await sut.save("test", testObj); + expect(sendUpdateSpy).not.toHaveBeenCalled(); + }); + + it("does not save the value a externally cached value exists which is an exact match", async () => { + sendMessageWithResponseSpy.mockResolvedValue(stringifiedTestObj); + await sut.save("test", testObj); + expect(sendUpdateSpy).not.toHaveBeenCalled(); + expect(cache["test"]).toBe(stringifiedTestObj); + }); + + it("saves the value if the currently cached string value evaluates to a falsy value", async () => { + cache["test"] = "null"; + await sut.save("test", testObj); + expect(sendUpdateSpy).toHaveBeenCalledWith({ key: "test", updateType: "save" }); + }); + }); + describe("caching", () => { beforeEach(() => { localStorageService.get.mockResolvedValue(null); @@ -167,21 +234,21 @@ describe("LocalBackedSessionStorage", () => { }); it("should remove key from cache if value is null", async () => { - cache.set("test", {}); - const cacheSetSpy = jest.spyOn(cache, "set"); - expect(cache.has("test")).toBe(true); + cache["test"] = {}; + // const cacheSetSpy = jest.spyOn(cache, "set"); + expect(cache["test"]).toBe(true); await sut.save("test", null); // Don't remove from cache, just replace with null - expect(cache.get("test")).toBe(null); - expect(cacheSetSpy).toHaveBeenCalledWith("test", null); + expect(cache["test"]).toBe(null); + // expect(cacheSetSpy).toHaveBeenCalledWith("test", null); }); it("should set cache if value is non-null", async () => { - expect(cache.has("test")).toBe(false); - const setSpy = jest.spyOn(cache, "set"); + expect(cache["test"]).toBe(false); + // const setSpy = jest.spyOn(cache, "set"); await sut.save("test", testObj); - expect(cache.get("test")).toBe(testObj); - expect(setSpy).toHaveBeenCalledWith("test", testObj); + expect(cache["test"]).toBe(stringifiedTestObj); + // expect(setSpy).toHaveBeenCalledWith("test", stringifiedTestObj); }); }); diff --git a/apps/browser/src/platform/services/local-backed-session-storage.service.ts b/apps/browser/src/platform/services/local-backed-session-storage.service.ts index 3f01e4169e9..146eb11b2bd 100644 --- a/apps/browser/src/platform/services/local-backed-session-storage.service.ts +++ b/apps/browser/src/platform/services/local-backed-session-storage.service.ts @@ -1,8 +1,10 @@ -import { Observable, Subject, filter, map, merge, share, tap } from "rxjs"; +import { Subject } from "rxjs"; import { Jsonify } from "type-fest"; import { EncryptService } from "@bitwarden/common/platform/abstractions/encrypt.service"; import { KeyGenerationService } from "@bitwarden/common/platform/abstractions/key-generation.service"; +import { LogService } from "@bitwarden/common/platform/abstractions/log.service"; +import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; import { AbstractMemoryStorageService, AbstractStorageService, @@ -13,57 +15,77 @@ import { EncString } from "@bitwarden/common/platform/models/domain/enc-string"; import { MemoryStorageOptions } from "@bitwarden/common/platform/models/domain/storage-options"; import { SymmetricCryptoKey } from "@bitwarden/common/platform/models/domain/symmetric-crypto-key"; -import { fromChromeEvent } from "../browser/from-chrome-event"; +import { BrowserApi } from "../browser/browser-api"; import { devFlag } from "../decorators/dev-flag.decorator"; import { devFlagEnabled } from "../flags"; +import { MemoryStoragePortMessage } from "../storage/port-messages"; +import { portName } from "../storage/port-name"; export class LocalBackedSessionStorageService extends AbstractMemoryStorageService implements ObservableStorageService { - private cache = new Map(); private updatesSubject = new Subject(); - - private commandName = `localBackedSessionStorage_${this.name}`; - private encKey = `localEncryptionKey_${this.name}`; - private sessionKey = `session_${this.name}`; - - updates$: Observable; + private commandName = `localBackedSessionStorage_${this.partitionName}`; + private encKey = `localEncryptionKey_${this.partitionName}`; + private sessionKey = `session_${this.partitionName}`; + private cachedSession: Record = {}; + private _ports: Set = new Set([]); + private knownNullishCacheKeys: Set = new Set([]); constructor( + private logService: LogService, private encryptService: EncryptService, private keyGenerationService: KeyGenerationService, private localStorage: AbstractStorageService, private sessionStorage: AbstractStorageService, - private name: string, + private platformUtilsService: PlatformUtilsService, + private partitionName: string, ) { super(); - const remoteObservable = fromChromeEvent(chrome.runtime.onMessage).pipe( - filter(([msg]) => msg.command === this.commandName), - map(([msg]) => msg.update as StorageUpdate), - tap((update) => { - if (update.updateType === "remove") { - this.cache.set(update.key, null); - } else { - this.cache.delete(update.key); - } - }), - share(), - ); + BrowserApi.addListener(chrome.runtime.onConnect, (port) => { + if (port.name !== `${portName(chrome.storage.session)}_${partitionName}`) { + return; + } - remoteObservable.subscribe(); + this._ports.add(port); - this.updates$ = merge(this.updatesSubject.asObservable(), remoteObservable); + const listenerCallback = this.onMessageFromForeground.bind(this); + port.onDisconnect.addListener(() => { + this._ports.delete(port); + port.onMessage.removeListener(listenerCallback); + }); + port.onMessage.addListener(listenerCallback); + // Initialize the new memory storage service with existing data + this.sendMessageTo(port, { + action: "initialization", + data: Array.from(Object.keys(this.cachedSession)), + }); + }); + this.updates$.subscribe((update) => { + this.broadcastMessage({ + action: "subject_update", + data: update, + }); + }); } get valuesRequireDeserialization(): boolean { return true; } + get updates$() { + return this.updatesSubject.asObservable(); + } + async get(key: string, options?: MemoryStorageOptions): Promise { - if (this.cache.has(key)) { - return this.cache.get(key) as T; + if (this.cachedSession[key] != null) { + return this.cachedSession[key] as T; + } + + if (this.knownNullishCacheKeys.has(key)) { + return null; } return await this.getBypassCache(key, options); @@ -71,7 +93,8 @@ export class LocalBackedSessionStorageService async getBypassCache(key: string, options?: MemoryStorageOptions): Promise { const session = await this.getLocalSession(await this.getSessionEncKey()); - if (session == null || !Object.keys(session).includes(key)) { + if (session[key] == null) { + this.knownNullishCacheKeys.add(key); return null; } @@ -80,8 +103,8 @@ export class LocalBackedSessionStorageService value = options.deserializer(value as Jsonify); } - this.cache.set(key, value); - return this.cache.get(key) as T; + void this.save(key, value); + return value as T; } async has(key: string): Promise { @@ -89,41 +112,48 @@ export class LocalBackedSessionStorageService } async save(key: string, obj: T): Promise { + // This is for observation purposes only. At some point, we don't want to write to local session storage if the value is the same. + if (this.platformUtilsService.isDev()) { + const existingValue = this.cachedSession[key] as T; + if (this.compareValues(existingValue, obj)) { + this.logService.warning(`Possible unnecessary write to local session storage. Key: ${key}`); + this.logService.warning(obj as any); + } + } + if (obj == null) { return await this.remove(key); } - this.cache.set(key, obj); + this.knownNullishCacheKeys.delete(key); + this.cachedSession[key] = obj; await this.updateLocalSessionValue(key, obj); - this.sendUpdate({ key, updateType: "save" }); + this.updatesSubject.next({ key, updateType: "save" }); } async remove(key: string): Promise { - this.cache.set(key, null); + this.knownNullishCacheKeys.add(key); + delete this.cachedSession[key]; await this.updateLocalSessionValue(key, null); - this.sendUpdate({ key, updateType: "remove" }); - } - - sendUpdate(storageUpdate: StorageUpdate) { - this.updatesSubject.next(storageUpdate); - void chrome.runtime.sendMessage({ - command: this.commandName, - update: storageUpdate, - }); + this.updatesSubject.next({ key, updateType: "remove" }); } private async updateLocalSessionValue(key: string, obj: T) { const sessionEncKey = await this.getSessionEncKey(); const localSession = (await this.getLocalSession(sessionEncKey)) ?? {}; localSession[key] = obj; - await this.setLocalSession(localSession, sessionEncKey); + void this.setLocalSession(localSession, sessionEncKey); } async getLocalSession(encKey: SymmetricCryptoKey): Promise> { - const local = await this.localStorage.get(this.sessionKey); + if (Object.keys(this.cachedSession).length > 0) { + return this.cachedSession; + } + this.cachedSession = {}; + const local = await this.localStorage.get(this.sessionKey); if (local == null) { - return null; + return this.cachedSession; } if (devFlagEnabled("storeSessionDecrypted")) { @@ -135,9 +165,11 @@ export class LocalBackedSessionStorageService // Error with decryption -- session is lost, delete state and key and start over await this.setSessionEncKey(null); await this.localStorage.remove(this.sessionKey); - return null; + return this.cachedSession; } - return JSON.parse(sessionJson); + + this.cachedSession = JSON.parse(sessionJson); + return this.cachedSession; } async setLocalSession(session: Record, key: SymmetricCryptoKey) { @@ -192,4 +224,76 @@ export class LocalBackedSessionStorageService await this.sessionStorage.save(this.encKey, input); } } + + private compareValues(value1: T, value2: T): boolean { + if (value1 == null && value2 == null) { + return true; + } + + if (value1 && value2 == null) { + return false; + } + + if (value1 == null && value2) { + return false; + } + + if (typeof value1 !== "object" || typeof value2 !== "object") { + return value1 === value2; + } + + if (JSON.stringify(value1) === JSON.stringify(value2)) { + return true; + } + + return Object.entries(value1).sort().toString() === Object.entries(value2).sort().toString(); + } + + private async onMessageFromForeground( + message: MemoryStoragePortMessage, + port: chrome.runtime.Port, + ) { + if (message.originator === "background") { + return; + } + + let result: unknown = null; + + switch (message.action) { + case "get": + case "getBypassCache": + case "has": { + result = await this[message.action](message.key); + break; + } + case "save": + await this.save(message.key, JSON.parse((message.data as string) ?? null) as unknown); + break; + case "remove": + await this.remove(message.key); + break; + } + + this.sendMessageTo(port, { + id: message.id, + key: message.key, + data: JSON.stringify(result), + }); + } + + protected broadcastMessage(data: Omit) { + this._ports.forEach((port) => { + this.sendMessageTo(port, data); + }); + } + + private sendMessageTo( + port: chrome.runtime.Port, + data: Omit, + ) { + port.postMessage({ + ...data, + originator: "background", + }); + } } diff --git a/apps/browser/src/platform/storage/foreground-memory-storage.service.ts b/apps/browser/src/platform/storage/foreground-memory-storage.service.ts index 1e5220002a8..b3ac8de55e3 100644 --- a/apps/browser/src/platform/storage/foreground-memory-storage.service.ts +++ b/apps/browser/src/platform/storage/foreground-memory-storage.service.ts @@ -21,12 +21,16 @@ export class ForegroundMemoryStorageService extends AbstractMemoryStorageService } updates$; - constructor() { + constructor(private partitionName?: string) { super(); this.updates$ = this.updatesSubject.asObservable(); - this._port = chrome.runtime.connect({ name: portName(chrome.storage.session) }); + let name = portName(chrome.storage.session); + if (this.partitionName) { + name = `${name}_${this.partitionName}`; + } + this._port = chrome.runtime.connect({ name }); this._backgroundResponses$ = fromChromeEvent(this._port.onMessage).pipe( map(([message]) => message), filter((message) => message.originator === "background"),