From e82eea6bcef6984bb5cbd58d418ba5e8a8867f8f Mon Sep 17 00:00:00 2001 From: Leslie Tilton <23057410+Banrion@users.noreply.github.com> Date: Fri, 23 Jan 2026 12:55:34 -0600 Subject: [PATCH] Fix meta data updates storing last checked instead of last updated --- .../services/phishing-data.service.spec.ts | 130 +++++++++++++++++- .../services/phishing-data.service.ts | 24 +++- 2 files changed, 146 insertions(+), 8 deletions(-) diff --git a/apps/browser/src/dirt/phishing-detection/services/phishing-data.service.spec.ts b/apps/browser/src/dirt/phishing-detection/services/phishing-data.service.spec.ts index b49e0e0e63a..9176e7209ba 100644 --- a/apps/browser/src/dirt/phishing-detection/services/phishing-data.service.spec.ts +++ b/apps/browser/src/dirt/phishing-detection/services/phishing-data.service.spec.ts @@ -10,7 +10,7 @@ import { import { FakeGlobalStateProvider } from "@bitwarden/common/spec"; import { LogService } from "@bitwarden/logging"; -import { PhishingDataService } from "./phishing-data.service"; +import { PHISHING_DOMAINS_META_KEY, PhishingDataService } from "./phishing-data.service"; import type { PhishingIndexedDbService } from "./phishing-indexeddb.service"; describe("PhishingDataService", () => { @@ -208,4 +208,132 @@ describe("PhishingDataService", () => { expect(meta.timestamp).toBeDefined(); }); }); + + describe("phishing meta data updates", () => { + it("should not update metadata when no data updates occur", async () => { + // Set up existing metadata + const existingMeta = { + checksum: "existing-checksum", + timestamp: Date.now() - 1000, // 1 second ago (not expired) + applicationVersion: "1.0.0", + }; + await fakeGlobalStateProvider.get(PHISHING_DOMAINS_META_KEY).update(() => existingMeta); + + // Mock conditions where no update is needed + fetchChecksumSpy.mockResolvedValue("existing-checksum"); // Same checksum + platformUtilsService.getApplicationVersion.mockResolvedValue("1.0.0"); // Same version + const mockResponse = { + ok: true, + body: {} as ReadableStream, + } as Response; + apiService.nativeFetch.mockResolvedValue(mockResponse); + + // Trigger background update + const result = await firstValueFrom(service["_backgroundUpdate"](existingMeta)); + + // Verify metadata was NOT updated (same reference returned) + expect(result).toEqual(existingMeta); + expect(result?.timestamp).toBe(existingMeta.timestamp); + + // Verify no data updates were performed + expect(mockIndexedDbService.saveUrlsFromStream).not.toHaveBeenCalled(); + expect(mockIndexedDbService.addUrls).not.toHaveBeenCalled(); + }); + + it("should update metadata when full dataset update occurs due to checksum change", async () => { + // Set up existing metadata + const existingMeta = { + checksum: "old-checksum", + timestamp: Date.now() - 1000, + applicationVersion: "1.0.0", + }; + await fakeGlobalStateProvider.get(PHISHING_DOMAINS_META_KEY).update(() => existingMeta); + + // Mock conditions for full update + fetchChecksumSpy.mockResolvedValue("new-checksum"); // Different checksum + platformUtilsService.getApplicationVersion.mockResolvedValue("1.0.0"); + const mockResponse = { + ok: true, + body: {} as ReadableStream, + } as Response; + apiService.nativeFetch.mockResolvedValue(mockResponse); + + // Trigger background update + const result = await firstValueFrom(service["_backgroundUpdate"](existingMeta)); + + // Verify metadata WAS updated with new values + expect(result?.checksum).toBe("new-checksum"); + expect(result?.timestamp).toBeGreaterThan(existingMeta.timestamp); + + // Verify full update was performed + expect(mockIndexedDbService.saveUrlsFromStream).toHaveBeenCalled(); + expect(mockIndexedDbService.addUrls).not.toHaveBeenCalled(); // Daily should not run + }); + + it("should update metadata when full dataset update occurs due to version change", async () => { + // Set up existing metadata + const existingMeta = { + checksum: "same-checksum", + timestamp: Date.now() - 1000, + applicationVersion: "1.0.0", + }; + await fakeGlobalStateProvider.get(PHISHING_DOMAINS_META_KEY).update(() => existingMeta); + + // Mock conditions for full update + fetchChecksumSpy.mockResolvedValue("same-checksum"); + platformUtilsService.getApplicationVersion.mockResolvedValue("2.0.0"); // Different version + const mockResponse = { + ok: true, + body: {} as ReadableStream, + } as Response; + apiService.nativeFetch.mockResolvedValue(mockResponse); + + // Trigger background update + const result = await firstValueFrom(service["_backgroundUpdate"](existingMeta)); + + // Verify metadata WAS updated + expect(result?.applicationVersion).toBe("2.0.0"); + expect(result?.timestamp).toBeGreaterThan(existingMeta.timestamp); + + // Verify full update was performed + expect(mockIndexedDbService.saveUrlsFromStream).toHaveBeenCalled(); + expect(mockIndexedDbService.addUrls).not.toHaveBeenCalled(); + }); + + it("should update metadata when daily update occurs due to cache expiration", async () => { + // Set up existing metadata (expired cache) + const existingMeta = { + checksum: "same-checksum", + timestamp: Date.now() - 25 * 60 * 60 * 1000, // 25 hours ago (expired) + applicationVersion: "1.0.0", + }; + await fakeGlobalStateProvider.get(PHISHING_DOMAINS_META_KEY).update(() => existingMeta); + + // Mock conditions for daily update only + fetchChecksumSpy.mockResolvedValue("same-checksum"); // Same checksum (no full update) + platformUtilsService.getApplicationVersion.mockResolvedValue("1.0.0"); // Same version + const mockFullResponse = { + ok: true, + body: {} as ReadableStream, + } as Response; + const mockDailyResponse = { + ok: true, + text: jest.fn().mockResolvedValue("newdomain.com"), + } as unknown as Response; + apiService.nativeFetch + .mockResolvedValueOnce(mockFullResponse) + .mockResolvedValueOnce(mockDailyResponse); + + // Trigger background update + const result = await firstValueFrom(service["_backgroundUpdate"](existingMeta)); + + // Verify metadata WAS updated + expect(result?.timestamp).toBeGreaterThan(existingMeta.timestamp); + expect(result?.checksum).toBe("same-checksum"); + + // Verify only daily update was performed + expect(mockIndexedDbService.saveUrlsFromStream).not.toHaveBeenCalled(); + expect(mockIndexedDbService.addUrls).toHaveBeenCalledWith(["newdomain.com"]); + }); + }); }); diff --git a/apps/browser/src/dirt/phishing-detection/services/phishing-data.service.ts b/apps/browser/src/dirt/phishing-detection/services/phishing-data.service.ts index a52002fc270..868aad3afad 100644 --- a/apps/browser/src/dirt/phishing-detection/services/phishing-data.service.ts +++ b/apps/browser/src/dirt/phishing-detection/services/phishing-data.service.ts @@ -338,26 +338,36 @@ export class PhishingDataService { () => { const appVersionChanged = newMeta.applicationVersion !== previous?.applicationVersion; const checksumChanged = newMeta.checksum !== previous?.checksum; + + this.logService.info( + `[PhishingDataService] Checking if full update is needed: appVersionChanged=${appVersionChanged}, checksumChanged=${checksumChanged}`, + ); return appVersionChanged || checksumChanged; }, - this._updateFullDataSet().pipe(map(() => newMeta)), - of(newMeta), + this._updateFullDataSet().pipe(map(() => ({ meta: newMeta, updated: true }))), + of({ meta: newMeta, updated: false }), ), ), // Update daily data set if last update was more than UPDATE_INTERVAL_DURATION ago - concatMap((newMeta) => + concatMap((result) => iif( () => { const isCacheExpired = Date.now() - (previous?.timestamp ?? 0) > this.UPDATE_INTERVAL_DURATION; return isCacheExpired; }, - this._updateDailyDataSet().pipe(map(() => newMeta)), - of(newMeta), + this._updateDailyDataSet().pipe(map(() => ({ meta: result.meta, updated: true }))), + of(result), ), ), - concatMap((newMeta) => { - return from(this._phishingMetaState.update(() => newMeta)).pipe( + concatMap((result) => { + if (!result.updated) { + this.logService.debug(`[PhishingDataService] No update needed, metadata unchanged`); + return of(previous); + } + + this.logService.debug(`[PhishingDataService] Updated phishing meta data:`, result.meta); + return from(this._phishingMetaState.update(() => result.meta)).pipe( tap(() => { const elapsed = Date.now() - startTime; this.logService.info(`[PhishingDataService] Updated in ${elapsed}ms`);