fix: drive background refresh and phishing update from alarms (closes #158)

src/shared/phishingDomains.js

@@ -8,8 +8,14 @@
 // The domain-checker checks the in-memory delta first (fresh/recent scam
 // sites), then falls back to the vendored list.
 //
-// If the delta is under 256 KiB it is persisted to localStorage so it
-// survives extension/service-worker restarts.
+// If the delta and its fetch timestamp fit in 256 KiB they are persisted to
+// extension storage, so they survive termination of the MV3 service worker.
+// Extension storage, not localStorage: localStorage does not exist in a
+// service worker, so the previous persistence never ran on Chrome at all.
+// The stored timestamps are what keep a restarted worker from re-fetching on
+// every wake while still noticing an overdue update. Those guards apply to the
+// startup path only; the 24-hour alarm tick bypasses them, or it would veto
+// its own refresh — see updatePhishingList().
 
 const vendoredConfig = require("./phishingBlocklist.json");
 
@@ -17,7 +23,14 @@ const BLOCKLIST_URL =
     "https://raw.githubusercontent.com/MetaMask/eth-phishing-detect/main/src/config.json";
 
 const CACHE_TTL_MS = 24 * 60 * 60 * 1000; // 24 hours
-const REFRESH_INTERVAL_MS = 24 * 60 * 60 * 1000; // 24 hours
+
+// Floor on how often an unscheduled path may hit the network. The worker is
+// revived every ~30 seconds while the browser is busy, and every revival runs
+// the startup path; without a persisted record of the last attempt, any state
+// that leaves lastFetchTime unset — a fetch that failed, or a delta too large
+// to store — would download the full list on every single wake.
+const MIN_FETCH_ATTEMPT_INTERVAL_MS = 60 * 60 * 1000; // 1 hour
+
 const DELTA_STORAGE_KEY = "phishing-delta";
 const MAX_DELTA_BYTES = 256 * 1024; // 256 KiB
 
@@ -29,45 +42,104 @@ const vendoredBlacklist = new Set(
 // Delta set — only entries from live list that are NOT in vendored.
 let deltaBlacklist = new Set();
 let lastFetchTime = 0;
+let lastAttemptTime = 0;
 let fetchPromise = null;
-let refreshTimer = null;
+let loadPromise = null;
+
+// Resolved on use rather than captured at module load, so a test can install
+// a stub after requiring the module and so the popup — which has no reason to
+// touch the delta — does not fail to load where the API is absent.
+function storageApi() {
+    if (typeof browser !== "undefined" && browser.storage) {
+        return browser.storage.local;
+    }
+    if (typeof chrome !== "undefined" && chrome.storage) {
+        return chrome.storage.local;
+    }
+    return null;
+}
 
 /**
- * Load delta entries from localStorage on startup.
- * Called once during module initialization in the background script.
+ * Sanitise a timestamp read back from storage.
+ *
+ * A value in the future is permanent poison: every guard here measures elapsed
+ * time as `Date.now() - stamp` and tests only the lower bound, so a stamp a
+ * year ahead suppresses updates for a year with no path that ever clears it.
+ * Clock skew and a restored profile backup both produce one. Since these
+ * timestamps only ever gate work, discarding an impossible one is safe: it
+ * costs at most a single extra fetch and restores a sane value immediately.
+ *
+ * @param {unknown} value
+ * @returns {number} the timestamp, or 0 if it is unusable.
  */
-function loadDeltaFromStorage() {
+function sanitizeTimestamp(value) {
+    if (typeof value !== "number" || !Number.isFinite(value)) return 0;
+    if (value <= 0 || value > Date.now()) return 0;
+    return value;
+}
+
+/**
+ * Load the persisted delta and its timestamps from extension storage.
+ * Runs once per worker lifetime; every entry point funnels through
+ * ensureDeltaLoaded() so a wake from termination restores state exactly once.
+ *
+ * @returns {Promise<void>}
+ */
+async function loadDeltaFromStorage() {
+    const storage = storageApi();
+    if (!storage) return;
     try {
-        const raw = localStorage.getItem(DELTA_STORAGE_KEY);
-        if (!raw) return;
-        const data = JSON.parse(raw);
-        if (data.blacklist && Array.isArray(data.blacklist)) {
+        const result = await storage.get(DELTA_STORAGE_KEY);
+        const data = result && result[DELTA_STORAGE_KEY];
+        if (!data) return;
+        if (Array.isArray(data.blacklist)) {
             deltaBlacklist = new Set(
                 data.blacklist.map((d) => d.toLowerCase()),
             );
         }
+        lastFetchTime = sanitizeTimestamp(data.lastFetchTime);
+        lastAttemptTime = sanitizeTimestamp(data.lastAttemptTime);
     } catch {
-        // localStorage unavailable or corrupt — start empty
+        // Storage unavailable or corrupt — start empty and re-fetch.
     }
 }
 
+function ensureDeltaLoaded() {
+    if (!loadPromise) loadPromise = loadDeltaFromStorage();
+    return loadPromise;
+}
+
 /**
- * Persist delta to localStorage if it fits within MAX_DELTA_BYTES.
+ * Persist the delta and its timestamps if they fit within MAX_DELTA_BYTES.
+ *
+ * The 256 KiB cap covers the delta and its freshness claim: when the delta is
+ * too large to keep, lastFetchTime goes with it, so the next start re-fetches
+ * rather than trusting a freshness claim for a delta it no longer holds.
+ * lastAttemptTime is written either way — it records that the network was
+ * contacted, which stays true whatever became of the response, and it is what
+ * stops a permanently oversized list from downloading on every worker wake.
+ *
+ * @returns {Promise<void>}
  */
-function saveDeltaToStorage() {
+async function saveDeltaToStorage() {
+    const storage = storageApi();
+    if (!storage) return;
     try {
         const data = {
             blacklist: Array.from(deltaBlacklist),
+            lastFetchTime,
+            lastAttemptTime,
         };
         const json = JSON.stringify(data);
         if (json.length < MAX_DELTA_BYTES) {
-            localStorage.setItem(DELTA_STORAGE_KEY, json);
+            await storage.set({ [DELTA_STORAGE_KEY]: data });
+        } else if (lastAttemptTime > 0) {
+            await storage.set({ [DELTA_STORAGE_KEY]: { lastAttemptTime } });
         } else {
-            // Too large — remove stale key if present
-            localStorage.removeItem(DELTA_STORAGE_KEY);
+            await storage.remove(DELTA_STORAGE_KEY);
         }
     } catch {
-        // localStorage unavailable — skip silently
+        // Storage unavailable — skip silently
     }
 }
 
@@ -76,6 +148,7 @@ function saveDeltaToStorage() {
  * Used for both live fetches and testing.
  *
  * @param {{ blacklist?: string[] }} config
+ * @returns {Promise<void>} resolves once the delta has been persisted.
  */
 function loadConfig(config) {
     const liveBlacklist = (config.blacklist || []).map((d) => d.toLowerCase());
@@ -86,7 +159,7 @@ function loadConfig(config) {
     );
 
     lastFetchTime = Date.now();
-    saveDeltaToStorage();
+    return saveDeltaToStorage();
 }
 
 /**
@@ -111,6 +184,11 @@ function hostnameVariants(hostname) {
  * Check if a hostname is on the phishing blocklist.
  * Checks delta first (fresh/recent scam sites), then vendored list.
  *
+ * Synchronous by design — callers answer an approval prompt with it. On a
+ * worker that has just woken, the persisted delta may still be loading; the
+ * vendored list, which is bundled and always present, carries the check until
+ * it lands.
+ *
  * @param {string} hostname - The hostname to check.
  * @returns {boolean}
  */
@@ -127,28 +205,59 @@ function isPhishingDomain(hostname) {
 
 /**
  * Fetch the latest blocklist and compute delta against vendored data.
- * De-duplicates concurrent fetches. Results are cached for CACHE_TTL_MS.
+ * De-duplicates concurrent fetches. Results are cached for CACHE_TTL_MS,
+ * counted from the persisted timestamp so the cache outlives the worker.
  *
+ * `force` is what makes the 24-hour alarm actually refresh every 24 hours.
+ * The alarm fires one period after the previous alarm, but lastFetchTime is
+ * stamped when that fetch *completed*, so an unforced tick lands one fetch
+ * latency inside its own TTL, skips, and turns the real cadence into 48 hours.
+ * Shortening the TTL instead would not fix it: the worker wakes every ~30
+ * seconds and the startup path re-checks the TTL each time, so a shortened TTL
+ * simply becomes the real cadence. The TTL is there to stop redundant fetches
+ * on wake, and the scheduled tick is not redundant, so it bypasses it.
+ *
+ * @param {{force?: boolean}} [opts] force: fetch unless one is already in
+ * flight, ignoring both the freshness and the retry guard. For the scheduled
+ * alarm tick only.
  * @returns {Promise<void>}
  */
-async function updatePhishingList() {
-    // Skip if recently fetched
-    if (Date.now() - lastFetchTime < CACHE_TTL_MS && lastFetchTime > 0) {
-        return;
+async function updatePhishingList({ force = false } = {}) {
+    // A worker that has just been revived knows nothing until the persisted
+    // record is back in memory; without this the freshness check below would
+    // always see 0 and re-fetch on every wake.
+    await ensureDeltaLoaded();
+
+    if (!force) {
+        const now = Date.now();
+        // Skip if recently fetched.
+        if (lastFetchTime > 0 && now - lastFetchTime < CACHE_TTL_MS) return;
+        // Skip if the network was contacted recently and the result was not
+        // usable — a failed fetch or an oversized delta leaves lastFetchTime
+        // unset, and without this every wake would retry.
+        if (
+            lastAttemptTime > 0 &&
+            now - lastAttemptTime < MIN_FETCH_ATTEMPT_INTERVAL_MS
+        ) {
+            return;
+        }
     }
 
     // De-duplicate concurrent calls
     if (fetchPromise) return fetchPromise;
 
     fetchPromise = (async () => {
+        lastAttemptTime = Date.now();
         try {
             const resp = await fetch(BLOCKLIST_URL);
             if (!resp.ok) throw new Error("HTTP " + resp.status);
             const config = await resp.json();
-            loadConfig(config);
+            await loadConfig(config);
         } catch {
-            // Silently fail — vendored list still provides coverage.
-            // We'll retry next time.
+            // Silently fail — vendored list still provides coverage. Persist
+            // the attempt so a persistently failing fetch is retried on the
+            // schedule rather than on every wake.
+            await saveDeltaToStorage();
         } finally {
             fetchPromise = null;
         }
@@ -158,12 +267,29 @@ async function updatePhishingList() {
 }
 
 /**
- * Start periodic refresh of the phishing list.
- * Should be called once from the background script on startup.
+ * Restore persisted state and fetch if the list is overdue.
+ *
+ * Called from the background script every time it starts — a fresh install,
+ * a browser start, and every revival of a terminated service worker all land
+ * here. The recurring 24-hour schedule itself is an alarm (see
+ * shared/alarms.js), not a timer, because timers die with the worker.
+ *
+ * @returns {Promise<void>}
  */
-function startPeriodicRefresh() {
-    if (refreshTimer) return;
-    refreshTimer = setInterval(updatePhishingList, REFRESH_INTERVAL_MS);
+async function initPhishingList() {
+    await ensureDeltaLoaded();
+    return updatePhishingList();
+}
+
+/**
+ * The 24-hour alarm tick. Separate from initPhishingList() because this is the
+ * scheduled refresh and must not be vetoed by the guards that exist to keep
+ * the unscheduled startup path off the network.
+ *
+ * @returns {Promise<void>}
+ */
+async function refreshPhishingListOnSchedule() {
+    return updatePhishingList({ force: true });
 }
 
 /**
@@ -190,21 +316,22 @@ function getDeltaSize() {
 function _reset() {
     deltaBlacklist = new Set();
     lastFetchTime = 0;
+    lastAttemptTime = 0;
     fetchPromise = null;
-    if (refreshTimer) {
-        clearInterval(refreshTimer);
-        refreshTimer = null;
-    }
+    loadPromise = null;
 }
 
-// Load persisted delta on module initialization
-loadDeltaFromStorage();
-
 module.exports = {
     isPhishingDomain,
     updatePhishingList,
-    startPeriodicRefresh,
+    refreshPhishingListOnSchedule,
+    initPhishingList,
+    loadDeltaFromStorage,
     loadConfig,
+    CACHE_TTL_MS,
+    MIN_FETCH_ATTEMPT_INTERVAL_MS,
+    DELTA_STORAGE_KEY,
+    MAX_DELTA_BYTES,
     getBlocklistSize,
     getDeltaSize,
     hostnameVariants,