From 240f595c68cd43be153743ed0c45d8662bbfee3f Mon Sep 17 00:00:00 2001 From: sidney-Hung Date: Fri, 6 Nov 2026 17:44:54 +0800 Subject: [PATCH 1/2] Start Practice when session storage is blocked Where site data is blocked, reading the sessionStorage global throws. The lobby's Start handler named the global in the arguments of its storage helpers, so the throw came before any helper could answer it, and the only trace was the browser's exception in an error line inside a collapsed section: Start looked like it did nothing. d5ad668 gave the interview page storageArea() for this, and the helpers already accept the null it answers, so Practice without selected grounding now reaches the interview, as the interview page expects. A server that requires sign-in cannot admit a candidate whose session cookie is refused, so when nobody is signed in Start stays on the lobby, says why in the account line and does not call /api/login. This assumes a blocked session storage means blocked cookies. Until the session answers Start waits, and a failed check offers the account retry instead of navigating, because either could hide such a server. A practice focus the candidate ticked to share is not carried: storeSharedFocus holds an unshared focus to be the safe outcome, and a refusal would land in the collapsed section. Selected grounding can only travel through storage, and Strict cannot run without it, so both stay on the lobby and that line now holds their own reason instead of the browser's exception. The section stays collapsed, so a Strict candidate still has to open it to read the reason. Change-Id: I811889775931b8bb8c1246e2446330ac2e8d0860 --- tests/browser/account.test.js | 7 +- tests/browser/lobby-render.test.js | 26 +- tests/browser/lobby.test.js | 382 +++++++++++++++++++++++++++++ web/app.js | 61 ++++- 4 files changed, 452 insertions(+), 24 deletions(-) diff --git a/tests/browser/account.test.js b/tests/browser/account.test.js index 39a257e3f..47bcc3cd3 100644 --- a/tests/browser/account.test.js +++ b/tests/browser/account.test.js @@ -309,7 +309,7 @@ test("optional interview profile is accessible, bounded, and omitted when blank" // The focus travels in session storage, never the address bar. assert.match( app, - /storeSharedFocus\(sessionStorage, focus\?\.weakness \?\? null\)/, + /const tabStorage = storageArea\("sessionStorage"\)[\s\S]*storeSharedFocus\(tabStorage, focus\?\.weakness \?\? null\)/, ); assert.doesNotMatch(app, /searchParams\.set\("focus"/); assert.match(interview, /practiceFocus: consumeSharedFocus\(tabStorage\)/); @@ -337,7 +337,10 @@ test("document grounding is explicit, clearable, ephemeral, and absent from save app, /nodes\.groundingClear\.addEventListener\("click", clearGrounding\)/, ); - assert.match(app, /storeGroundingPacket\(sessionStorage, packet\)/); + assert.match( + app, + /const tabStorage = storageArea\("sessionStorage"\)[\s\S]*storeGroundingPacket\(tabStorage, packet\)/, + ); assert.match(interview, /consumeGroundingPacket\(tabStorage\)/); const savedArtifacts = [ read("history.js"), diff --git a/tests/browser/lobby-render.test.js b/tests/browser/lobby-render.test.js index b13767829..25bee5def 100644 --- a/tests/browser/lobby-render.test.js +++ b/tests/browser/lobby-render.test.js @@ -19,7 +19,7 @@ // depends on: `document.querySelectorAll` answers ids and nothing else, so the // problem cards, the difficulty radios, the duration buttons and the loop // buttons are all absent, and this drives the page that is left. What that page -// still proves is the import, the signed-out account state, and the branch the +// still proves is the import, the unidentified account state, and the branch the // progress panel takes when it cannot read anything. import { test } from "node:test"; import assert from "node:assert/strict"; @@ -33,24 +33,28 @@ const dom = installDocument(markup); // work while it loads. Every test below reads the same rendered document. await import("/app.js"); -/// What the page may say to a signed-out visitor whose device has no history. +/// What the page may say to a visitor it cannot identify, on a device with no +/// history. /// /// Both of those are the stub's doing and neither is contrived: `installDocument` /// refuses the fetch the page starts at import, so the session lookup fails, and -/// plain `node` has no `localStorage`, so the saved-progress read fails too. -/// That is the worst case a real visitor can also reach, on a first visit with -/// the network down, and it is the one state this file can produce without a -/// browser. -const SIGNED_OUT = { - "account-status": "Signed out", +/// plain `node` has no storage globals, so the saved-progress read fails too. +/// That is the worst case a real visitor can also reach, with site data blocked +/// and the network down, and it is the one state this file can produce without a +/// browser. Start is held there, because a server that requires sign-in would +/// send the visitor straight back, so the page offers the account retry. +const UNIDENTIFIED = { + "account-status": "Could not check your account. Retry the account check.", + "login-link": "Retry account check", "progress-problem": "all", start: "Start interview", - "progress-summary": "Could not load progress saved on this device.", + "progress-summary": + "Could not load account progress. Retry the account check.", "practice-list-status": "0 problems in your list. Open the problem picker below to check problems. Kept for this visit only; browser storage is unavailable.", }; test("the lobby page loads outside a browser and says what it was given", () => { - for (const [id, expected] of Object.entries(SIGNED_OUT)) { + for (const [id, expected] of Object.entries(UNIDENTIFIED)) { assert.deepEqual(dom.node(id).spoken(), [expected], `#${id}`); } }); @@ -64,7 +68,7 @@ test("no other panel of the lobby speaks before anything has loaded", () => { const spoke = []; for (const [, id] of markup.matchAll(/id="([^"]+)"/g)) { const node = dom.node(id); - if (node && node.spoken().length && !(id in SIGNED_OUT)) spoke.push(id); + if (node && node.spoken().length && !(id in UNIDENTIFIED)) spoke.push(id); } assert.deepEqual( spoke, diff --git a/tests/browser/lobby.test.js b/tests/browser/lobby.test.js index 7bf2df76f..45ecbdaf3 100644 --- a/tests/browser/lobby.test.js +++ b/tests/browser/lobby.test.js @@ -561,6 +561,388 @@ lobbyTest( }, ); +// Where site data is blocked, reading the `sessionStorage` global throws. Start +// named the global in the arguments of its storage helpers, so the throw came +// before any helper could answer it, and the candidate was left on the lobby. +const blockSessionStorage = (page) => + page.addInitScript(() => { + Object.defineProperty(window, "sessionStorage", { + get() { + throw new DOMException("blocked", "SecurityError"); + }, + }); + }); + +lobbyTest("Practice starts when session storage is blocked", async (page) => { + await blockSessionStorage(page); + const picked = (await lobby(page)).card; + await page.click("#start"); + await page.waitForURL("**/interview?**"); + assert.equal(new URL(page.url()).searchParams.get("problem"), picked); +}); + +// Unlike grounding, a ticked focus does not stop Start: `storeSharedFocus` holds +// an unshared focus to be the safe outcome, and a refusal would land in the +// collapsed section, leaving a Start that looks dead. The address bar must not +// become the way round, since anyone can craft a link carrying that text. +lobbyTest( + "Practice starts without a shared focus when session storage is blocked", + async (page) => { + await blockSessionStorage(page); + reports = [focusedAttempt(EASY[0])]; + const state = await lobby(page); + assert.match(state.focus, /Test boundaries/); + await page.check("#practice-focus-share-input"); + await page.click("#start"); + await page.waitForURL("**/interview?**"); + assert.equal(new URL(page.url()).searchParams.get("problem"), state.card); + assert.doesNotMatch( + decodeURIComponent(page.url()).replaceAll("+", " "), + /Test boundaries/, + ); + }, +); + +// The packet can only reach the interview page through the tab's storage, so +// starting without it would drop what the candidate chose. Clearing is the way +// out the message offers, and it has to work with storage still blocked. +lobbyTest( + "Practice with grounding offers clearing it when session storage is blocked", + async (page) => { + await blockSessionStorage(page); + await lobby(page); + await page.click("details.interview-context summary"); + await page.setInputFiles("#grounding-jd", { + name: "jd.txt", + mimeType: "text/plain", + buffer: Buffer.from("Must know Rust"), + }); + await page + .locator('#grounding-choices input[data-group="requirements"]') + .first() + .check(); + await page.check("#grounding-consent"); + await page.click("#start"); + await page.waitForFunction( + () => document.querySelector("#grounding-error").textContent !== "", + ); + assert.match( + await page.locator("#grounding-error").textContent(), + /Clear grounding to start normally/, + ); + assert.equal(new URL(page.url()).pathname, "/"); + await page.click("#grounding-clear"); + await page.click("#start"); + await page.waitForURL("**/interview?**"); + }, +); + +// Where the block also refuses the session cookie, a recorded username would +// reach the interview signed out and come straight back with its choices reset. +// Phone-sized, because there Start sits far below the account row the reason +// lands in, and a reason out of view is a Start that looks dead. +lobbyTest( + "Start asks for site data before signing in when session storage is blocked", + async (page) => { + await blockSessionStorage(page); + session = { + signedIn: false, + loginRequired: true, + guestModeAvailable: false, + }; + let logins = 0; + page.on("request", (request) => { + if (new URL(request.url()).pathname === "/api/login") logins++; + }); + await lobby(page); + await page.fill("#github-login", "candidate"); + await page.click("details.interview-context summary"); + await page.setInputFiles("#grounding-jd", { + name: "jd.txt", + mimeType: "text/plain", + buffer: Buffer.from("Must know Rust"), + }); + await page + .locator('#grounding-choices input[data-group="requirements"]') + .first() + .check(); + await page.click("#start"); + assert.match( + await page.locator("#grounding-error").textContent(), + /Agree to send/, + ); + await page.check("#grounding-consent"); + await page.click("#start"); + await reaches(page, () => + document + .querySelector("#account-status") + .textContent.includes("Signing in needs site data"), + ); + assert.match( + await page.locator("#account-status").textContent(), + /Signing in needs site data/, + ); + // The refusal the candidate just answered does not stay beside the reason. + assert.equal(await page.locator("#grounding-error").textContent(), ""); + assert.equal( + await page.locator("#account-status").evaluate((node) => { + const box = node.getBoundingClientRect(); + return box.top >= 0 && box.bottom <= window.innerHeight; + }), + true, + ); + assert.equal(new URL(page.url()).pathname, "/"); + assert.equal(logins, 0); + assert.equal(await page.isDisabled("#start"), false); + }, + { viewport: { width: 375, height: 700 } }, +); + +// Until the session answers, the guard above cannot tell a server that requires +// sign-in from one that admits guests, so Start waits rather than guessing. +lobbyTest( + "Start waits for the session when session storage is blocked", + async (page) => { + await blockSessionStorage(page); + session = { + signedIn: false, + loginRequired: true, + guestModeAvailable: false, + }; + let releaseSession; + holdSession = new Promise((resolve) => (releaseSession = resolve)); + try { + const sessionRequest = page.waitForRequest( + (request) => new URL(request.url()).pathname === "/api/session", + ); + await page.goto(`${base}/`, { waitUntil: "load" }); + await sessionRequest; + await page.click("details.problem-picker summary"); + await page.click(`[data-problem="${MEDIUM[0]}"]`); + assert.equal(await page.isDisabled("#start"), true); + await page.evaluate(() => { + document.querySelector("#start").dispatchEvent(new MouseEvent("click")); + }); + await normalPageshow(page); + assert.equal(new URL(page.url()).pathname, "/"); + } finally { + releaseSession(); + } + await awaitReady(page); + assert.equal(new URL(page.url()).pathname, "/"); + assert.equal( + (await page.locator("#start").textContent()).trim(), + "Use GitHub to start", + ); + assert.equal(await page.isDisabled("#start"), false); + }, +); + +// A failed check cannot be waved through here as it is where storage works: a +// server that requires sign-in would send the candidate straight back. +lobbyTest( + "Start offers the account retry when the check fails and storage is blocked", + async (page) => { + await blockSessionStorage(page); + session = { + signedIn: false, + loginRequired: true, + guestModeAvailable: false, + }; + failing.add("/api/session"); + await lobby(page); + assert.equal( + await page.locator("#account-status").textContent(), + "Could not check your account. Retry the account check.", + ); + // Nobody signed in, so there is nothing to sign out of. + assert.equal(await page.locator("#logout").isHidden(), true); + assert.equal(await page.isDisabled("#start"), true); + await page.evaluate(() => { + document.querySelector("#start").dispatchEvent(new MouseEvent("click")); + }); + await normalPageshow(page); + assert.equal(new URL(page.url()).pathname, "/"); + + failing.delete("/api/session"); + await page.click("#login-link"); + await awaitReady(page); + assert.equal( + (await page.locator("#start").textContent()).trim(), + "Use GitHub to start", + ); + assert.equal(await page.isDisabled("#start"), false); + }, +); + +// No login lies behind this retry, so its answer must not read as a failed one. +lobbyTest( + "a retry on a guest server does not ask for a sign-in that never happened", + async (page) => { + await blockSessionStorage(page); + session = { + signedIn: false, + loginRequired: true, + guestModeAvailable: true, + }; + let logins = 0; + page.on("request", (request) => { + if (new URL(request.url()).pathname === "/api/login") logins++; + }); + failing.add("/api/session"); + await lobby(page); + failing.delete("/api/session"); + await page.click("#login-link"); + await awaitReady(page); + assert.doesNotMatch( + await page.locator("#account-status").textContent(), + /Try signing in again/, + ); + assert.equal(logins, 0); + assert.equal(await page.isDisabled("#start"), false); + }, +); + +// The answer that matters most: a retry that finds the candidate signed in has +// to clear the failed check, or Start and every delete stay down for good. +lobbyTest( + "a signed-in answer to the retry releases Start when storage is blocked", + async (page) => { + await blockSessionStorage(page); + failing.add("/api/session"); + await lobby(page); + assert.equal(await page.isDisabled("#start"), true); + failing.delete("/api/session"); + await page.click("#login-link"); + await awaitReady(page); + assert.equal(await page.isDisabled("#start"), false); + assert.match( + await page.locator("#account-status").textContent(), + /Signed in as candidate/, + ); + }, +); + +// Coming back from a Start that left: the retry state must not keep showing the +// label of a start that is no longer on its way. +lobbyTest( + "a restored lobby drops Starting when the check fails and storage is blocked", + async (page) => { + await blockSessionStorage(page); + session = { signedIn: false, loginRequired: false }; + await lobby(page); + // Put there rather than pressed: a navigation held open stalls every later + // step of the test, and only what the restore does with the label is at stake. + await page.evaluate(() => { + document.querySelector("#start").textContent = "Starting..."; + }); + failing.add("/api/session"); + await restore(page); + assert.equal( + await reaches(page, () => + document + .querySelector("#account-status") + .textContent.includes("Could not check your account"), + ), + true, + ); + assert.equal( + (await page.locator("#start").textContent()).trim(), + "Start interview", + ); + }, +); + +// A login the server accepted stays known as recorded: only the account check +// after it failed, and the candidate should not be told the sign-in is in doubt. +lobbyTest( + "a recorded login stays recorded when the check fails and storage is blocked", + async (page) => { + await blockSessionStorage(page); + session = { + signedIn: false, + loginRequired: true, + guestModeAvailable: false, + }; + await page.route("**/api/login", (route) => { + failing.add("/api/session"); + return route.fulfill({ json: { ok: true } }); + }); + await lobby(page); + await page.fill("#github-login", "candidate"); + await page.click("#login-link"); + await reaches(page, () => + document + .querySelector("#account-status") + .textContent.includes("account could not be refreshed"), + ); + assert.match( + await page.locator("#account-status").textContent(), + /GitHub username recorded, but the account could not be refreshed/, + ); + }, +); + +// The hold is for blocked storage alone: elsewhere Practice may still start on +// a manual pick before the account answers. +lobbyTest( + "Start does not wait for the session when storage works", + async (page) => { + session = { + signedIn: false, + loginRequired: true, + guestModeAvailable: false, + }; + let releaseSession; + holdSession = new Promise((resolve) => (releaseSession = resolve)); + try { + const sessionRequest = page.waitForRequest( + (request) => new URL(request.url()).pathname === "/api/session", + ); + await page.goto(`${base}/`, { waitUntil: "load" }); + await sessionRequest; + await page.click("details.problem-picker summary"); + await page.click(`[data-problem="${MEDIUM[0]}"]`); + assert.equal(await page.isDisabled("#start"), false); + } finally { + releaseSession(); + } + }, +); + +// A signed-out answer is an answer: where guests are admitted, blocked storage +// is no reason to offer a retry, and Start goes on to the interview. +lobbyTest( + "a guest starts when the session answers and storage is blocked", + async (page) => { + await blockSessionStorage(page); + session = { signedIn: false, loginRequired: false }; + await lobby(page); + assert.equal(await page.isDisabled("#start"), false); + await page.click("#start"); + await page.waitForURL("**/interview?**"); + }, +); + +// Strict cannot run without the tab's storage, so it stays on the lobby. What is +// pinned is that the reason is the one selection.js wrote for the candidate, not +// the browser's own exception text. +lobbyTest("Strict names session storage when it is blocked", async (page) => { + await blockSessionStorage(page); + await lobby(page); + await page.selectOption("#selection-mode", "strict"); + await page.click("#start"); + await page.waitForFunction( + () => document.querySelector("#grounding-error").textContent !== "", + ); + assert.match( + await page.locator("#grounding-error").textContent(), + /Strict Interview needs session storage/, + ); + assert.equal(new URL(page.url()).pathname, "/"); + assert.equal(await page.isDisabled("#start"), false); +}); + lobbyTest( "editor options default to all five features enabled", async (page) => { diff --git a/web/app.js b/web/app.js index f7ce345a2..1b0467d13 100644 --- a/web/app.js +++ b/web/app.js @@ -115,6 +115,9 @@ let loginPending = false; // Successful and uncertain POSTs need an account check before another login. // Null means no check is owed; otherwise this is "recorded" or "unknown". let loginResult = null; +// A session check that failed where storage is blocked. It holds Start like the +// above and is retried the same way, but no POST lies behind it. +let accountCheckFailed = false; let deletingReports = false; /// The longest interview this deployment can record, which only the server /// knows. `Infinity` until `/api/session` answers: the duration row is live @@ -766,6 +769,7 @@ start.addEventListener("click", async () => { (!historyReady || accountHistory !== true)) || starting || accountUpdatePending() || + sessionUnknownWithoutStorage() || deletingReports ) return; @@ -819,9 +823,20 @@ start.addEventListener("click", async () => { return; } + nodes.groundingError.textContent = ""; + // Assumes a block on storage also refuses the session cookie, where signing in + // here would reach the interview signed out and come straight back. A block on + // storage alone can still sign in through Use GitHub, which stays ungated. + if (signInFirst && !storageArea("sessionStorage")) { + nodes.accountStatus.textContent = + "Signing in needs site data, which this browser is blocking for this site. Allow site data for this site, then reload this page."; + // On a narrow page Start sits far below the account row, out of its view. + nodes.githubLogin.focus(); + return; + } + starting = true; syncLobbyControls(); - nodes.groundingError.textContent = ""; if (signInFirst) { start.textContent = "Recording GitHub..."; if (!(await recordGitHubLogin())) { @@ -840,9 +855,10 @@ start.addEventListener("click", async () => { start.textContent = "Starting..."; if (!nodes.groundingConsent.checked) packet = null; try { - if (strict) resetAssignments(sessionStorage); - storeGroundingPacket(sessionStorage, packet); - storeSharedFocus(sessionStorage, focus?.weakness ?? null); + const tabStorage = storageArea("sessionStorage"); + if (strict) resetAssignments(tabStorage); + storeGroundingPacket(tabStorage, packet); + storeSharedFocus(tabStorage, focus?.weakness ?? null); } catch (error) { nodes.groundingError.textContent = error.message; starting = false; @@ -921,7 +937,7 @@ function clearGrounding() { nodes.groundingError.textContent = ""; nodes.groundingChoices.replaceChildren(); try { - storeGroundingPacket(sessionStorage, null); + storeGroundingPacket(storageArea("sessionStorage"), null); } catch { /* clearing is best effort */ } @@ -970,7 +986,7 @@ nodes.loginLink.addEventListener("click", async () => { syncLobbyControls(); try { // Retrying an account check must not create another recorded session. - if (!loginResult) { + if (!loginResult && !accountCheckFailed) { if (!(await recordGitHubLogin())) return; loginResult = "recorded"; } @@ -993,6 +1009,7 @@ function syncLobbyControls() { (!historyReady || accountHistory !== true)) || starting || accountBusy || + sessionUnknownWithoutStorage() || deletingReports; selection.disabled = starting; nodes.loginLink.disabled = @@ -1020,7 +1037,20 @@ function syncLobbyControls() { } function accountUpdatePending() { - return loginPending || loginResult !== null || historyRefreshing; + return ( + loginPending || + loginResult !== null || + accountCheckFailed || + historyRefreshing + ); +} + +/// Practice may start before the account answers, except where storage is +/// blocked: the answer then decides whether Start can sign in at all. +function sessionUnknownWithoutStorage() { + return ( + !historyReady && accountHistory === null && !storageArea("sessionStorage") + ); } /// One rule for the bulk delete and every row's: until the lobby knows whose @@ -1119,6 +1149,7 @@ async function loadAccount(generation) { throw new Error("Invalid account response"); } accountHistory = false; + accountCheckFailed = false; const peerLink = document.getElementById("peer-practice"); if (peerLink) peerLink.hidden = !( @@ -1176,7 +1207,14 @@ async function loadAccount(generation) { } } if (generation !== historyLoad) return; - if (loginResult) { + // Not waved through as the catch above says: with storage blocked, a server + // that requires sign-in would send the candidate straight back. Retry is GET. + if (accountHistory === null && !storageArea("sessionStorage")) { + accountCheckFailed = loginResult === null; + // A restored page may still be labelled for the start that left it. + setStartGate(signInFirst); + } + if (loginResult || accountCheckFailed) { showAccountCheckError(); return; } @@ -1189,14 +1227,15 @@ async function loadAccount(generation) { } function showAccountCheckError() { - nodes.accountStatus.textContent = - loginResult === "recorded" + nodes.accountStatus.textContent = accountCheckFailed + ? "Could not check your account. Retry the account check." + : loginResult === "recorded" ? "GitHub username recorded, but the account could not be refreshed." : "Could not confirm the sign-in result. Retry the account check."; nodes.githubLogin.hidden = true; nodes.loginLink.hidden = false; nodes.loginLink.textContent = "Retry account check"; - nodes.logout.hidden = false; + nodes.logout.hidden = accountCheckFailed; showProgressError( "Could not load account progress. Retry the account check.", ); From 58ec809328c18a4e127b86ebdafe16487dfcead4 Mon Sep 17 00:00:00 2001 From: sidney-Hung Date: Sat, 7 Nov 2026 14:01:52 +0800 Subject: [PATCH 2/2] 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: I8fddb84b7ff95dc4a0e507b2d5b1b522ba9154f7 --- eslint.config.mjs | 40 +++++++++++++++++++++++++++++++++++ tests/browser/account.test.js | 32 ++++++++++++++++++++++++++++ web/audio-output.js | 4 ++-- web/history.js | 26 +++++++++++++++++------ 4 files changed, 93 insertions(+), 9 deletions(-) diff --git a/eslint.config.mjs b/eslint.config.mjs index f86f1dd7a..ee66f1dfd 100644 --- a/eslint.config.mjs +++ b/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. // diff --git a/tests/browser/account.test.js b/tests/browser/account.test.js index 47bcc3cd3..49b51602c 100644 --- a/tests/browser/account.test.js +++ b/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. diff --git a/web/audio-output.js b/web/audio-output.js index 6bbf644cb..706ce6919 100644 --- a/web/audio-output.js +++ b/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]; diff --git a/web/history.js b/web/history.js index 6f9c6532a..062bb84f6 100644 --- a/web/history.js +++ b/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