Reject direct use of the browser storage globals

Naming localStorage or sessionStorage throws where site data is
blocked, and works everywhere else, so the bug passes every local run:
it stopped the interview page from loading until d5ad668 and left the
lobby's Start doing nothing visible. The lint gate now refuses the
names under web/ and points at storageArea(), which answers null
instead. web/history.js moves to it through a helper that throws when
no store can be reached, because its callers tell a failed delete from
an empty store by that throw: each catch still reports the failure, so
its results do not change. web/peer-lobby.js is left out with the
reason beside the exclusion: it has the same failure, at module scope
when opened from an invite, and fixing it means choosing where an
invite token lives without storage, not a rename.

Close #600

Change-Id: I12ba2f40fc04adbc12f42bc8fe99a8acfe67c9ba

Author: sidney-Hung

eslint.config.mjs

@@ -63,6 +63,33 @@ const CORRECTNESS = {
   "no-async-promise-executor": "error",
 };
 
+// Where site data is blocked, touching either storage global throws rather than
+// answering a storage that refuses, and the name alone is enough: at module
+// scope in `web/interview.js` it stopped the whole page from loading (d5ad668),
+// and as an argument it throws before the callee's own `try` can answer it. Both
+// work wherever storage is allowed, so a local run never shows either failure.
+const STORAGE_MESSAGE =
+  'Throws where site data is blocked. Use storageArea() from "./audio-output.js".';
+const STORAGE_ACCESS = {
+  "no-restricted-globals": [
+    "error",
+    ...["localStorage", "sessionStorage"].map((name) => ({
+      name,
+      message: STORAGE_MESSAGE,
+    })),
+  ],
+  "no-restricted-properties": [
+    "error",
+    ...["window", "self", "globalThis"].flatMap((object) =>
+      ["localStorage", "sessionStorage"].map((property) => ({
+        object,
+        property,
+        message: STORAGE_MESSAGE,
+      })),
+    ),
+  ],
+};
+
 export default [
   {
     // `externals/` holds unrelated checkouts, is untracked, and is excluded in
@@ -84,6 +111,19 @@ export default [
     linterOptions: { reportUnusedDisableDirectives: "error" },
     rules: CORRECTNESS,
   },
+  {
+    // Application code only. The review workbench under `.claude/skills` is a
+    // developer tool, not a page a candidate opens.
+    //
+    // `web/peer-lobby.js` is exempt for now. Three of its five
+    // references sit outside any `try`, one of them at module scope, and a
+    // mechanical `storageArea(...).setItem` would pass this rule and still throw
+    // on null. The fix needs a decision about where the invite token lives
+    // without storage, after the page has removed it from the URL.
+    files: ["web/**/*.js"],
+    ignores: ["web/peer-lobby.js"],
+    rules: STORAGE_ACCESS,
+  },
   {
     // Node: the browser tests and the generator scripts.
     //

tests/browser/account.test.js

@@ -1220,6 +1220,38 @@ test("a report the account kept stays on this device too", async () => {
   );
 });
 
+// A device copy that cannot be reached is a failed delete, not a missing report:
+// the lobby tells the candidate what these answers say.
+test("deleting a report says so when local storage cannot be reached", async () => {
+  const original = Object.getOwnPropertyDescriptor(globalThis, "localStorage");
+  Object.defineProperty(globalThis, "localStorage", {
+    configurable: true,
+    get() {
+      throw new DOMException("blocked", "SecurityError");
+    },
+  });
+  try {
+    assert.equal(
+      await deleteReport("keep", {
+        fetcher: async () => response({ signedIn: false }),
+      }),
+      "failed",
+    );
+    assert.equal(
+      await deleteReport("keep", {
+        fetcher: async (url) =>
+          url === "/api/session"
+            ? response({ signedIn: true })
+            : response({ deleted: 1 }),
+      }),
+      "account-deleted-local-failed",
+    );
+  } finally {
+    if (original) Object.defineProperty(globalThis, "localStorage", original);
+    else delete globalThis.localStorage;
+  }
+});
+
 test("deleting an id no store holds says so instead of claiming a removal", async () => {
   // A pre-id row whose assigned id never reached storage: the rows are still
   // there, so answering "deleted" named a removal the next read undoes.

web/audio-output.js

@@ -163,8 +163,8 @@ function showOutputNote(text) {
 /// there, touching either global throws rather than returning a storage that
 /// refuses. The helpers below default through this rather than naming the
 /// global, because a default parameter is evaluated before their `try`, and
-/// every reader of the result already treats a failing call as "nothing
-/// stored", which `null` is.
+/// they treat a failing call as "nothing stored", which `null` is. A caller
+/// that must tell a failure from an empty store has to check for null itself.
 export function storageArea(name) {
   try {
     return globalThis[name];

web/history.js

@@ -1,3 +1,5 @@
+import { storageArea } from "./audio-output.js";
+
 export const historyKey = "codetrial_history";
 export const reviewHistoryKey = "codetrial_review_history";
 
@@ -14,6 +16,16 @@ const REVIEW_HISTORY_BYTES = 164 * 1024;
 /// A deadline turns both into the failure they already know how to report.
 const requestTimeoutMs = 10_000;
 
+/// The store a caller passed, or this device's, and a throw where neither can be
+/// reached. `storageArea` answers null there, and `storedList` reads a null store
+/// as "no list stored", so carrying it on would let `deleteReport` claim a removal
+/// it never ran. The throw reaches each caller's catch, as the bare global's did.
+function deviceStorage(storage) {
+  const store = storage || storageArea("localStorage");
+  if (!store) throw new Error("local storage is unavailable");
+  return store;
+}
+
 /// Always a list. The key sits in the origin's local storage, which an older
 /// build, another tab, or anyone with devtools open can write, so what it holds
 /// is input rather than something this module chose. Unparseable JSON was
@@ -34,7 +46,7 @@ export function readLocalHistory(storage) {
 /// not a list, and the shape check already turns that into one.
 function storedList(key, storage) {
   try {
-    const stored = JSON.parse((storage || localStorage).getItem(key));
+    const stored = JSON.parse(deviceStorage(storage).getItem(key));
     return Array.isArray(stored) ? stored : null;
   } catch {
     return null;
@@ -43,7 +55,7 @@ function storedList(key, storage) {
 
 export function readReviewHistory(storage) {
   try {
-    storage ||= localStorage;
+    storage = deviceStorage(storage);
     const stored = storedList(reviewHistoryKey, storage);
     if (stored) return stored;
     const { retained, json } = boundedReviews(readLocalHistory(storage));
@@ -73,7 +85,7 @@ export function readReviewHistory(storage) {
 /// deduplicated within a store quietly merged them.
 export function readDeviceHistory(storage) {
   try {
-    storage ||= localStorage;
+    storage = deviceStorage(storage);
     const history = readLocalHistory(storage);
     const reviews = readReviewHistory(storage);
     // Mutates both arrays, and writes both stores when it has anything to say.
@@ -194,7 +206,7 @@ function legacyKey(entry) {
 /// from the history when it is first read, after this has run.
 export function renameLocalHistory(pages, storage, markUnmapped = false) {
   try {
-    storage ||= localStorage;
+    storage = deviceStorage(storage);
     const history = renamedEntries(
       readLocalHistory(storage),
       pages,
@@ -301,7 +313,7 @@ export async function clearReportHistory({
       return "failed";
     }
     try {
-      storage ||= localStorage;
+      storage = deviceStorage(storage);
       storage.removeItem(historyKey);
       storage.removeItem(reviewHistoryKey);
       return "cleared";
@@ -348,7 +360,7 @@ export async function deleteReport(
     }
     let removed = fromAccount;
     try {
-      storage ||= localStorage;
+      storage = deviceStorage(storage);
       // A missing review store is left missing: it is rebuilt from the short
       // history on its first read, and that no longer holds the report.
       for (const key of [historyKey, reviewHistoryKey]) {
@@ -374,7 +386,7 @@ export async function deleteReport(
 
 function saveLocalReport(entry, storage) {
   try {
-    storage ||= localStorage;
+    storage = deviceStorage(storage);
     const previous = readLocalHistory(storage);
     // The short history first, and before the review store is even read.
     // Reading it can rebuild it, and that write is the 164 KB one: spending the