Make the persistent-storage request observable and warn when it is denied (#33987)
* Make the persistent-storage request observable and warn when it is denied The end-to-end encryption crypto store lives in IndexedDB. If the origin's storage is not durable, Chromium can evict it under storage pressure, forcing a logout and recovery-key re-entry. tryPersistStorage() requested navigator.storage.persist() but only logged the boolean result, so a denial was invisible. Make the request observable: it is now async and checks persisted() first (short-circuiting to avoid re-requesting/re-prompting), a failure to query the state no longer blocks the request, and a denial warns via the logger (captured by rageshakes) with a stronger desktop-specific message. It never rejects - the sole caller treats it as fire-and-forget. This makes the risk observable but cannot by itself guarantee durability: no Electron main-process API can force per-origin persistence, so a complete cure needs a follow-up. * Address review: log query errors, drop the requestStorageAccess fallback and the desktop-specific warning - Include the caught error when the persisted-state query fails. - Remove the document.requestStorageAccess branch: it is the Storage Access API (cross-site cookie/storage access), not durability, and all supported browsers (Safari >= 15.2 included) have navigator.storage.persist(). - Drop the desktop-specific suffix from the denial warning. - Reword the fire-and-forget doc to avoid referencing the caller. - Simplify the test harness: no descriptor save/restore, delete the stub in afterEach.
This commit is contained in:
hayyaksi
@@ -22,23 +22,72 @@ function log(msg: string): void {
|
||||
logger.log(`StorageManager: ${msg}`);
|
||||
}
|
||||
|
||||
function warn(msg: string, ...args: any[]): void {
|
||||
logger.warn(`StorageManager: ${msg}`, ...args);
|
||||
}
|
||||
|
||||
function error(msg: string, ...args: any[]): void {
|
||||
logger.error(`StorageManager: ${msg}`, ...args);
|
||||
}
|
||||
|
||||
export function tryPersistStorage(): void {
|
||||
if (navigator.storage && navigator.storage.persist) {
|
||||
navigator.storage.persist().then((persistent) => {
|
||||
logger.log("StorageManager: Persistent?", persistent);
|
||||
});
|
||||
} else if (document.requestStorageAccess) {
|
||||
// Safari
|
||||
document.requestStorageAccess().then(
|
||||
() => logger.log("StorageManager: Persistent?", true),
|
||||
() => logger.log("StorageManager: Persistent?", false),
|
||||
);
|
||||
} else {
|
||||
logger.log("StorageManager: Persistence unsupported");
|
||||
/**
|
||||
* Warn (in the logs, captured by rageshakes) that the browser refused to make our storage
|
||||
* persistent. Without durable storage the browser may evict IndexedDB — which holds the
|
||||
* end-to-end encryption crypto store — under storage pressure, forcing a re-login and
|
||||
* recovery-key re-entry. See https://github.com/element-hq/element-web/issues/32198.
|
||||
*
|
||||
* We deliberately do NOT surface a user-facing dialog/toast here: on a packaged desktop
|
||||
* (custom-scheme) build Chromium's durable-storage heuristic commonly returns `false` even
|
||||
* in the healthy case, so a per-login warning would be a false-alarm flood. The actual
|
||||
* post-eviction user prompt is handled separately by {@link checkConsistency} →
|
||||
* StorageEvictedDialog.
|
||||
*/
|
||||
function warnPersistenceDenied(): void {
|
||||
warn(
|
||||
"Persistent storage was not granted. The browser may evict locally stored data " +
|
||||
"(including the end-to-end encryption keys in the crypto store) under storage pressure, " +
|
||||
"which can force a re-login. See https://github.com/element-hq/element-web/issues/32198.",
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* Ask the browser to make our storage persistent (durable), so it is not evicted under
|
||||
* storage pressure. Acts on the result: warns when persistence is denied.
|
||||
*
|
||||
* Invoked on every login *and* session restore, so we first check whether storage is
|
||||
* already persistent and short-circuit to avoid re-requesting (some browsers re-prompt).
|
||||
*
|
||||
* Never rejects, so it is safe to call fire-and-forget.
|
||||
*
|
||||
* @returns whether storage is persistent after the attempt.
|
||||
*/
|
||||
export async function tryPersistStorage(): Promise<boolean> {
|
||||
try {
|
||||
if (navigator.storage && navigator.storage.persist) {
|
||||
// Avoid re-requesting (and possibly re-prompting) when we already have it. A failure
|
||||
// to *query* the state must not stop us from *requesting* persistence below.
|
||||
try {
|
||||
if (navigator.storage.persisted && (await navigator.storage.persisted())) {
|
||||
log("Persistent storage already granted");
|
||||
return true;
|
||||
}
|
||||
} catch (e) {
|
||||
warn("Could not query persisted-storage state; requesting persistence anyway", e);
|
||||
}
|
||||
const persistent = await navigator.storage.persist();
|
||||
log(`Persistent? ${persistent}`);
|
||||
if (!persistent) {
|
||||
warnPersistenceDenied();
|
||||
}
|
||||
return persistent;
|
||||
} else {
|
||||
log("Persistence unsupported");
|
||||
return false;
|
||||
}
|
||||
} catch (e) {
|
||||
// A storage-API hiccup must never reject into the fire-and-forget caller.
|
||||
error("Failed to request persistent storage", e);
|
||||
return false;
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -10,6 +10,7 @@ import "fake-indexeddb/auto";
|
||||
|
||||
import { IDBFactory } from "fake-indexeddb";
|
||||
import { IndexedDBCryptoStore } from "matrix-js-sdk/src/matrix";
|
||||
import { logger } from "matrix-js-sdk/src/logger";
|
||||
|
||||
import * as StorageManager from "../../../src/utils/StorageManager";
|
||||
|
||||
@@ -119,4 +120,92 @@ describe("StorageManager", () => {
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
describe("tryPersistStorage", () => {
|
||||
// jsdom does not implement navigator.storage, so stub it per-test; jest.replaceProperty
|
||||
// cannot be used as it refuses to replace a property that does not exist.
|
||||
function setStorage(value: unknown): void {
|
||||
Object.defineProperty(navigator, "storage", { value, configurable: true });
|
||||
}
|
||||
|
||||
beforeEach(() => {
|
||||
jest.spyOn(logger, "log").mockImplementation(() => {});
|
||||
jest.spyOn(logger, "warn").mockImplementation(() => {});
|
||||
jest.spyOn(logger, "error").mockImplementation(() => {});
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
delete (navigator as unknown as { storage?: unknown }).storage;
|
||||
jest.restoreAllMocks();
|
||||
});
|
||||
|
||||
it("returns true and does not re-request when storage is already persisted", async () => {
|
||||
const persist = jest.fn().mockResolvedValue(true);
|
||||
const persisted = jest.fn().mockResolvedValue(true);
|
||||
setStorage({ persist, persisted });
|
||||
|
||||
await expect(StorageManager.tryPersistStorage()).resolves.toBe(true);
|
||||
expect(persisted).toHaveBeenCalled();
|
||||
expect(persist).not.toHaveBeenCalled();
|
||||
expect(logger.warn).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("requests persistence and returns true when granted", async () => {
|
||||
const persist = jest.fn().mockResolvedValue(true);
|
||||
const persisted = jest.fn().mockResolvedValue(false);
|
||||
setStorage({ persist, persisted });
|
||||
|
||||
await expect(StorageManager.tryPersistStorage()).resolves.toBe(true);
|
||||
expect(persist).toHaveBeenCalled();
|
||||
expect(logger.warn).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("requests persistence directly when persisted() is unavailable", async () => {
|
||||
const persist = jest.fn().mockResolvedValue(true);
|
||||
setStorage({ persist });
|
||||
|
||||
await expect(StorageManager.tryPersistStorage()).resolves.toBe(true);
|
||||
expect(persist).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it("still requests persistence and logs the failure when querying the persisted state fails", async () => {
|
||||
const queryError = new Error("query failed");
|
||||
const persisted = jest.fn().mockRejectedValue(queryError);
|
||||
const persist = jest.fn().mockResolvedValue(true);
|
||||
setStorage({ persist, persisted });
|
||||
|
||||
await expect(StorageManager.tryPersistStorage()).resolves.toBe(true);
|
||||
expect(persist).toHaveBeenCalled();
|
||||
expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining("Could not query"), queryError);
|
||||
});
|
||||
|
||||
it("returns false and warns when persistence is denied", async () => {
|
||||
const persist = jest.fn().mockResolvedValue(false);
|
||||
const persisted = jest.fn().mockResolvedValue(false);
|
||||
setStorage({ persist, persisted });
|
||||
|
||||
await expect(StorageManager.tryPersistStorage()).resolves.toBe(false);
|
||||
expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining("Persistent storage"));
|
||||
});
|
||||
|
||||
it("returns false when navigator.storage lacks persist()", async () => {
|
||||
setStorage({ persisted: jest.fn().mockResolvedValue(false) });
|
||||
|
||||
await expect(StorageManager.tryPersistStorage()).resolves.toBe(false);
|
||||
expect(logger.log).toHaveBeenCalledWith(expect.stringContaining("unsupported"));
|
||||
});
|
||||
|
||||
it("returns false without throwing when persistence is unsupported", async () => {
|
||||
await expect(StorageManager.tryPersistStorage()).resolves.toBe(false);
|
||||
});
|
||||
|
||||
it("does not reject but logs an error if requesting persistence throws", async () => {
|
||||
const persist = jest.fn().mockRejectedValue(new Error("boom"));
|
||||
const persisted = jest.fn().mockResolvedValue(false);
|
||||
setStorage({ persist, persisted });
|
||||
|
||||
await expect(StorageManager.tryPersistStorage()).resolves.toBe(false);
|
||||
expect(logger.error).toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user