From 3d9ef039e5d28a28d26de78b65de5d0317c9f3eb Mon Sep 17 00:00:00 2001 From: ScreenTinker Date: Thu, 6 Aug 2026 16:18:25 -0500 Subject: [PATCH 1/2] Make the volume slider real, and stop an empty playlist wiping the cache MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three faults in the web player, each found by driving the shipped code in a browser against the real server rather than by reading it. set_volume did nothing at all. The dashboard sends `{ level: 0..1 }` (device-detail.js: slider/100) and the Android player reads exactly that; this player read `payload.value` and divided it by 100. Nothing in the product sends `value`, so every browser panel acked the command and ignored it — the quietest possible failure. Correcting only the key would have been worse than leaving it broken: `level: 0.5` would have become 0.5%, which is inaudible and looks fixed. The fraction is now canonical, `value` is still read as a percentage for anything written against the old handler, and the scale is chosen by WHICH KEY arrived rather than by the size of the number — 1 is legal in both conventions, so a magnitude guess is guaranteed to be wrong for somebody. Parsing moved into volumeLevelFromCommand() so it can be asserted without a socket. setMediaVolume() also wrote `el.muted = (v === 0)`, so any non-zero volume un-muted whatever was playing. An item an operator had deliberately silenced started making noise the moment anyone touched the slider — reproduced live: item flagged muted, one set_volume, muted went false. Mute has four inputs and a fixed order (lib/media-mute.js), it is resolved when the element is mounted, and a level is not entitled to overrule it — least of all the autoplay rule, where unmuting without a gesture costs the video rather than winning the audio. Volume 0 is silence on its own. And the service worker pruned its content cache to an EMPTY keep-set. `assignments: []` is what the server sends for a device between playlists, for a playlist never published, and inside the `catch` when a published_snapshot fails to parse — none of which mean "delete the media". Reproduced: three cached assets, one empty payload, cache emptied. That is only survivable while the uplink is up, which is precisely when the offline cache is worthless. A cache kept too long costs disk the quota reclaims anyway. Verified in Chrome against a live server: volume 0.42/0.8/0/0.25 land on the element and survive an item change, a muted item stays muted through a volume command, and three cached assets survive an empty push. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Uaeo9MvzKoyXuN6ZsbhtkL --- server/player/index.html | 61 +++++++- server/player/sw.js | 19 ++- server/test/player-sw-scope.test.js | 7 +- server/test/player-volume-command.test.js | 182 ++++++++++++++++++++++ server/test/sw-prune-guard.test.js | 123 +++++++++++++++ 5 files changed, 383 insertions(+), 9 deletions(-) create mode 100644 server/test/player-volume-command.test.js create mode 100644 server/test/sw-prune-guard.test.js diff --git a/server/player/index.html b/server/player/index.html index 88c0f0d..35b61fc 100644 --- a/server/player/index.html +++ b/server/player/index.html @@ -1676,11 +1676,12 @@ if (BS && BS.reboot()) console.log('[bs] reboot requested via host'); else console.log('reboot: not supported on this player'); } - // Media volume, 0-100 from the dashboard. Applies to whatever is playing now and is - // remembered for items mounted later. + // Media volume. Applies to whatever is playing now and is remembered for items mounted + // later (see setMediaVolume). The wire parsing is volumeLevelFromCommand(). if (data.type === 'set_volume') { - const pct = Number(data.payload?.value ?? data.value); - if (isFinite(pct)) setMediaVolume(Math.max(0, Math.min(100, pct)) / 100); + const v = volumeLevelFromCommand(data); + if (v === null) console.warn('[volume] set_volume with no usable level/value:', JSON.stringify(data)); + else setMediaVolume(v); } }); @@ -4032,6 +4033,42 @@ try { return BS.displayPower(on); } catch (e) { return false; } } + /* + * The 0..1 volume a set_volume command is asking for, or null when it does not carry one. + * + * The wire form is `payload.level`, a FRACTION. That is what the dashboard sends + * (device-detail.js: `{ level: slider / 100 }`), what a group command relays unchanged, and what + * the Android player reads (`optDouble("level")`) — so the fraction is canonical and this player + * now matches it instead of inventing a third convention. + * + * What was here read `payload.value` and treated it as a percentage, which matched nothing the + * product sends: the slider was a no-op on every browser panel. Correcting only the KEY would + * have been worse than leaving it broken — 0.5 clamped as a percentage is 0.5%, inaudible, and + * it would have looked fixed to anyone who checked only that the handler ran. + * + * `value` is still accepted as a 0..100 percentage for any caller written against the old + * handler. The scale is chosen by WHICH KEY ARRIVED, never by the magnitude of the number: 1 is + * legal in both scales (1% and full volume), so a magnitude test is guaranteed to be wrong for + * somebody, silently, in whichever direction hurts more. + * + * Extracted from the socket handler so the parsing can be asserted without a socket — the bug + * it replaces was invisible precisely because nothing ever ran it against a real payload. + */ + function volumeLevelFromCommand(data) { + if (!data || typeof data !== 'object') return null; + const p = (data.payload && typeof data.payload === 'object') ? data.payload : {}; + const pick = (k) => { + const raw = (p[k] !== undefined && p[k] !== null) ? p[k] : data[k]; + if (raw === undefined || raw === null || raw === '' || typeof raw === 'boolean') return null; + const n = Number(raw); + return isFinite(n) ? n : null; + }; + const level = pick('level'); // canonical: 0..1 + const pct = level === null ? pick('value') : null; // legacy: 0..100 + const v = level !== null ? level : (pct !== null ? pct / 100 : null); + return v === null ? null : Math.max(0, Math.min(1, v)); + } + // Volume that survives the next item. Media elements are created per item, so remembering the // level is what makes a volume command stick rather than lasting until the playlist advances. let mediaVolume = null; @@ -4040,8 +4077,17 @@ // Wall followers stay silent by design; don't override that. try { if (typeof isWallFollower === 'function' && isWallFollower()) return; } catch (e) { /* not a wall */ } document.querySelectorAll('video, audio').forEach((el) => { - try { el.volume = v; el.muted = v === 0; } catch (e) { /* element torn down mid-call */ } + try { el.volume = v; } catch (e) { /* element torn down mid-call */ } }); + // Volume is a LEVEL. Mute is a separate decision with four inputs and a fixed order + // (lib/media-mute.js), already resolved when the element was mounted, and this function does + // not get to overrule it. It used to write `el.muted = (v === 0)`, which un-muted whatever was + // playing on any non-zero volume: an item an operator had deliberately silenced started making + // noise the moment somebody touched the volume slider — reproduced live (item flagged muted, + // one set_volume, muted went false). The same write contradicts the resolver's autoplay rule, + // where unmuting without a user gesture costs the VIDEO rather than winning the audio. + // + // Nothing is lost by leaving mute alone: volume 0 is silence on every media element we run on. } // Media elements are created per item across several code paths — fullscreen, zones, the @@ -4059,7 +4105,10 @@ } if (mediaVolume == null) return; try { if (typeof isWallFollower === 'function' && isWallFollower()) return; } catch (err) { /* not a wall */ } - try { el.volume = mediaVolume; el.muted = mediaVolume === 0; } catch (err) { /* gone */ } + // Level only — see setMediaVolume: the mount path has already resolved this element's mute + // from media-mute.js, and re-deciding it here from the volume number alone would undo a + // per-item mute (and, without a user gesture, the playback itself). + try { el.volume = mediaVolume; } catch (err) { /* gone */ } }, true); // Screen-off state, tracked because the playlist keeps advancing while the screen is "off" diff --git a/server/player/sw.js b/server/player/sw.js index 806f8a2..c369ae4 100644 --- a/server/player/sw.js +++ b/server/player/sw.js @@ -1,3 +1,6 @@ +// v24: an empty playlist payload no longer prunes. `assignments: []` is what the server sends for a +// device between playlists AND for a snapshot that failed to parse, and treating it as "keep +// nothing" wiped the panel's entire offline library on a message that means nothing of the sort. // v23: offline.cache is claimed only when a worker is actually IN CONTROL — a real BrightSign // widget exposes navigator.serviceWorker, refuses to register one, and was advertising the // capability to the fleet regardless. @@ -11,7 +14,7 @@ // — a player then ran a new index.html against a stale st-bridge.js and threw on every heartbeat. // Bump whenever a shipped /player asset changes shape; content lives in its own cache, so this // costs a small re-download and never re-fetches the playlist. -const CACHE_NAME = 'rd-player-v23'; +const CACHE_NAME = 'rd-player-v24'; // Content lives in its own cache so the shell can be re-versioned (the activate handler deletes // every cache that is not CACHE_NAME) WITHOUT throwing away megabytes of media that are still // perfectly valid. Rolling the shell used to mean a player re-downloaded its entire playlist. @@ -143,7 +146,19 @@ self.addEventListener('message', (event) => { // writes a new randomly-named file, so the old copy lives at a different PATH and nothing keyed // on the asset path can find it. Without this the cache only grows, and on a panel with a 1GB // widget quota a handful of replaced videos is the entire budget. - if (data.prune) prefetchChain = prefetchChain.then(() => pruneToPlaylist(data.urls)).catch(() => {}); + // + // An EMPTY list is never a prune instruction, and that distinction is the whole guard. "This + // display needs nothing" and "the payload did not arrive intact" are the same message on the + // wire, and the second one is not rare: buildPlaylistPayload() yields `assignments: []` for a + // device between playlists, for a playlist that has never been published, AND for a + // published_snapshot that fails to JSON.parse. Honouring it deleted every byte of media the panel + // held — reproduced here: three cached assets, one empty payload, cache emptied — which is only + // survivable while the uplink is up, i.e. exactly when this cache does not matter. A cache that is + // kept too long costs disk the quota reclaims anyway; one dropped at the wrong moment is a dark + // screen with no way back. + if (data.prune && data.urls.length > 0) { + prefetchChain = prefetchChain.then(() => pruneToPlaylist(data.urls)).catch(() => {}); + } for (const url of data.urls) { if (typeof url !== 'string' || !POLICY || !POLICY.isCacheableContent(url, 'GET')) continue; diff --git a/server/test/player-sw-scope.test.js b/server/test/player-sw-scope.test.js index 8e4e353..7dee238 100644 --- a/server/test/player-sw-scope.test.js +++ b/server/test/player-sw-scope.test.js @@ -70,7 +70,12 @@ test('the worker prunes to the set the player declares', () => { // a panel with a 1GB widget quota, a few replaced videos is the whole budget. const sw = fs.readFileSync(path.join(__dirname, '..', 'player', 'sw.js'), 'utf8'); assert.match(sw, /function pruneToPlaylist/); - assert.match(sw, /if \(data\.prune\)/, 'the prune must be driven by the player declaring a complete set'); + // Driven by the player declaring a complete set — and an EMPTY set is not a declaration. See + // test/sw-prune-guard.test.js, which runs the handler: `assignments: []` is what the server sends + // for a device between playlists and for a snapshot that failed to parse, and honouring it as + // "keep nothing" wiped the panel's whole offline library. + assert.match(sw, /if \(data\.prune && data\.urls\.length > 0\)/, + 'the prune must be driven by the player declaring a NON-EMPTY complete set'); const html = fs.readFileSync(path.join(__dirname, '..', 'player', 'index.html'), 'utf8'); assert.match(html, /prune:\s*true/); diff --git a/server/test/player-volume-command.test.js b/server/test/player-volume-command.test.js new file mode 100644 index 0000000..00b683c --- /dev/null +++ b/server/test/player-volume-command.test.js @@ -0,0 +1,182 @@ +'use strict'; + +// The web player's set_volume handler, and the mute state it is not allowed to touch. +// +// Two bugs lived here at once, and the second is why fixing the first alone would have been worse +// than leaving it broken: +// +// 1. THE KEY. The dashboard sends `{ level: <0..1> }` (device-detail.js: slider/100) and the +// Android player reads exactly that (`optDouble("level")`). The web player read +// `payload.value`. Nothing in the product sends `value`, so the slider was a silent no-op on +// every browser panel — and a no-op is invisible: the command is delivered, acked, and does +// nothing. +// 2. THE SCALE. The local was named `pct` and divided by 100. Had only the key been corrected, +// `level: 0.5` would have become 0.5% — near-silence that LOOKS like the fix worked, because +// the handler now runs and the element's volume genuinely changes. +// +// And separately: setMediaVolume() wrote `el.muted = (v === 0)`, so any non-zero volume command +// un-muted whatever was playing — defeating a per-item mute an operator had set on purpose +// (reproduced in a browser: item flagged muted, one set_volume, muted went false), and defying the +// autoplay rule in lib/media-mute.js, where unmuting without a gesture costs the video. +// +// The real functions are extracted from index.html and run against fake globals, so this asserts +// what the shipped player does rather than a paraphrase of it. + +const { test } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); +const vm = require('node:vm'); + +const HTML = fs.readFileSync(path.join(__dirname, '..', 'player', 'index.html'), 'utf8'); + +/** Extract a top-level `function name(...) { ... }` from the player by brace matching. */ +function extract(name) { + const start = HTML.indexOf(` function ${name}(`); + assert.notEqual(start, -1, `${name}() must exist in the player`); + let depth = 0; + for (let i = HTML.indexOf('{', start); i < HTML.length; i++) { + if (HTML[i] === '{') depth++; + else if (HTML[i] === '}' && --depth === 0) return HTML.slice(start, i + 1); + } + throw new Error(`unterminated ${name}()`); +} + +function loadVolumeParser() { + const sandbox = { console: { log() {}, warn() {} } }; + vm.createContext(sandbox); + vm.runInContext(extract('volumeLevelFromCommand'), sandbox); + return (data) => vm.runInContext('volumeLevelFromCommand', sandbox)(data); +} + +/** + * setMediaVolume() against fake media elements. Returns the elements so the test can see exactly + * which properties were written. + */ +function loadSetVolume({ wallFollower = false, elements } = {}) { + const els = elements || [ + { tagName: 'VIDEO', volume: 1, muted: false }, + { tagName: 'VIDEO', volume: 1, muted: true }, + ]; + const sandbox = { + console: { log() {}, warn() {} }, + document: { querySelectorAll: () => els }, + isWallFollower: () => wallFollower, + mediaVolume: null, + }; + sandbox.window = sandbox; + vm.createContext(sandbox); + vm.runInContext('let mediaVolume = null;\n' + extract('setMediaVolume'), sandbox); + return { els, call: (v) => vm.runInContext('setMediaVolume', sandbox)(v), sandbox }; +} + +// --------------------------------------------------------------------------------------------- +// THE WIRE FORM + +test('THE BUG: the key the dashboard actually sends is honoured', () => { + const parse = loadVolumeParser(); + assert.equal(parse({ type: 'set_volume', payload: { level: 0.5 } }), 0.5); +}); + +test('THE OTHER HALF: a fraction is not divided by 100', () => { + // The trap. 0.5 -> 0.005 is near-mute and would read as "fixed" to anyone checking that the + // handler fires at all. + const parse = loadVolumeParser(); + assert.equal(parse({ type: 'set_volume', payload: { level: 0.5 } }), 0.5); + assert.equal(parse({ type: 'set_volume', payload: { level: 1 } }), 1); + assert.equal(parse({ type: 'set_volume', payload: { level: 0 } }), 0); +}); + +test('the legacy percentage key is still understood, as a percentage', () => { + const parse = loadVolumeParser(); + assert.equal(parse({ type: 'set_volume', payload: { value: 80 } }), 0.8); + assert.equal(parse({ type: 'set_volume', value: 25 }), 0.25); // top-level, as the old code read it +}); + +test('the scale comes from the KEY, never from the magnitude', () => { + // 1 is legal in both conventions — full volume as a fraction, 1% as a percentage. A magnitude + // heuristic has to be wrong for one of them, so there is no heuristic. + const parse = loadVolumeParser(); + assert.equal(parse({ payload: { level: 1 } }), 1, 'level:1 is FULL volume'); + assert.equal(parse({ payload: { value: 1 } }), 0.01, 'value:1 is one percent'); +}); + +test('level wins when a caller sends both', () => { + const parse = loadVolumeParser(); + assert.equal(parse({ payload: { level: 0.3, value: 90 } }), 0.3); +}); + +test('out-of-range input is clamped, not wrapped', () => { + const parse = loadVolumeParser(); + assert.equal(parse({ payload: { level: 4 } }), 1); + assert.equal(parse({ payload: { level: -2 } }), 0); + assert.equal(parse({ payload: { value: 5000 } }), 1); +}); + +test('a command carrying no level is refused rather than turned into 0', () => { + // Returning 0 for "no value" would silence a display on a malformed command — a failure that + // looks exactly like someone dragging the slider down. + const parse = loadVolumeParser(); + for (const bad of [{}, { payload: {} }, { payload: { level: null } }, { payload: { level: '' } }, + { payload: { level: 'loud' } }, { payload: { level: true } }, null, undefined]) { + assert.equal(parse(bad), null, `expected null for ${JSON.stringify(bad)}`); + } +}); + +test('numeric strings are accepted — an integrator posting JSON as text still works', () => { + const parse = loadVolumeParser(); + assert.equal(parse({ payload: { level: '0.4' } }), 0.4); + assert.equal(parse({ payload: { value: '70' } }), 0.7); +}); + +// --------------------------------------------------------------------------------------------- +// WHAT IT IS NOT ALLOWED TO TOUCH + +test('THE BUG: a volume command does not un-mute a deliberately muted item', () => { + const { els, call } = loadSetVolume(); + els[1].muted = true; // an operator silenced this item + call(0.6); + assert.equal(els[1].volume, 0.6, 'the level still applies'); + assert.equal(els[1].muted, true, 'a muted element must stay muted'); + assert.equal(els[0].muted, false, 'and an unmuted one must stay unmuted'); +}); + +test('volume 0 does not need to set muted — the level alone is silence', () => { + const { els, call } = loadSetVolume(); + call(0); + assert.equal(els[0].volume, 0); + assert.equal(els[0].muted, false, 'mute is not this function\'s decision to make'); +}); + +test('a wall follower is left silent — volume never reaches its elements', () => { + // One audio source per wall. A follower that honoured the slider would give the room the same + // track from every panel, a few milliseconds apart. + const { els, call } = loadSetVolume({ wallFollower: true }); + call(0.9); + assert.equal(els[0].volume, 1, 'untouched'); +}); + +test('the level is remembered so it survives the next item', () => { + const { call, sandbox } = loadSetVolume(); + call(0.35); + assert.equal(vm.runInContext('mediaVolume', sandbox), 0.35); +}); + +test('an element torn down mid-call cannot break the loop', () => { + const boom = { tagName: 'VIDEO', set volume(v) { throw new Error('detached'); }, muted: false }; + const good = { tagName: 'VIDEO', volume: 1, muted: false }; + const { call } = loadSetVolume({ elements: [boom, good] }); + call(0.5); + assert.equal(good.volume, 0.5, 'the surviving element still gets the level'); +}); + +// --------------------------------------------------------------------------------------------- +// THE HANDLER WIRING — the parser is useless if the socket handler does not call it + +test('the set_volume handler routes through the parser and setMediaVolume', () => { + const handler = HTML.slice(HTML.indexOf("if (data.type === 'set_volume')")); + const block = handler.slice(0, handler.indexOf('\n }') + 10); + assert.match(block, /volumeLevelFromCommand\(data\)/, 'the handler must use the shared parser'); + assert.match(block, /setMediaVolume\(/, 'and apply it'); + assert.ok(!/\/\s*100/.test(block), 'the handler must not re-scale — the parser already returns 0..1'); +}); diff --git a/server/test/sw-prune-guard.test.js b/server/test/sw-prune-guard.test.js new file mode 100644 index 0000000..cef5fb3 --- /dev/null +++ b/server/test/sw-prune-guard.test.js @@ -0,0 +1,123 @@ +'use strict'; + +// What the worker does with the playlist message the player posts it — specifically the prune half. +// +// THE BUG: `pruneToPlaylist` deletes every content entry not in the keep-set, and the message +// handler ran it on an EMPTY keep-set. `assignments: []` is not a rare shape. buildPlaylistPayload() +// produces it for a device between playlists, for a playlist that has never been published, and — +// this is the one that hurts — inside `catch (e) { assignments = []; }` when a published_snapshot +// fails to JSON.parse. Any of those wiped every byte of media the panel had cached, which is only +// survivable while the uplink is up, i.e. exactly when the offline cache does not matter. +// Reproduced in a browser before the fix: three cached assets, one empty payload, cache emptied. +// +// Runs the SHIPPED worker (player/sw.js) against a fake Cache API, capturing the message listener +// it registers — the listener is the thing under test, so stubbing addEventListener away (as the +// prefetch tests do) would leave this path untested, which is how it shipped. + +const { test } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); +const vm = require('node:vm'); + +const SW_SRC = fs.readFileSync(path.join(__dirname, '..', 'player', 'sw.js'), 'utf8'); +const POLICY_PATH = path.join(__dirname, '..', 'lib', 'player-cache-policy.js'); + +const A = 'http://s/uploads/content/a.mp4?rev=1'; +const B = 'http://s/uploads/content/b.png?rev=1'; +const OLD = 'http://s/uploads/content/a.mp4?rev=0'; + +class FakeCache { + constructor() { this.map = new Map(); } + #url(req) { return typeof req === 'string' ? req : req.url; } + async match(req) { const r = this.map.get(this.#url(req)); return r ? r.clone() : undefined; } + async put(req, res) { this.map.set(this.#url(req), res); } + async keys() { return [...this.map.keys()].map((u) => ({ url: u })); } + async delete(req) { return this.map.delete(this.#url(req)); } +} + +function load() { + const content = new FakeCache(); + const listeners = {}; + const sandbox = { + caches: { + open: async () => content, + keys: async () => ['rd-content-v1'], + delete: async () => true, + match: async () => undefined, + }, + // Any network use here would be a bug in the test, not the worker: prune touches no network. + fetch: async () => { throw new Error('prune must not fetch'); }, + Response, Request, Blob, URL, console, + location: { href: 'http://s/player/index.html' }, + navigator: {}, + importScripts() { + delete require.cache[require.resolve(POLICY_PATH)]; + sandbox.self.PlayerCachePolicy = require(POLICY_PATH); + }, + addEventListener(type, fn) { (listeners[type] = listeners[type] || []).push(fn); }, + skipWaiting() {}, + clients: { claim() {} }, + }; + sandbox.self = sandbox; + vm.createContext(sandbox); + vm.runInContext(SW_SRC, sandbox); + return { sandbox, content, post: (data) => listeners.message.forEach((fn) => fn({ data })) }; +} + +const seed = async (content, urls) => { + for (const u of urls) await content.put(u, new Response('x')); +}; +const keys = (content) => [...content.map.keys()].sort(); +// The worker chains prune onto its serialised prefetch queue, so give the microtasks a turn. +const settle = () => new Promise((r) => setTimeout(r, 20)); + +test('THE BUG: an empty playlist must NOT wipe the offline cache', async () => { + const { content, post } = load(); + await seed(content, [A, B]); + post({ type: 'st-cache-playlist', urls: [], prune: true }); + await settle(); + assert.deepEqual(keys(content), [A, B].sort(), + 'a payload with no assignments is indistinguishable from a payload that failed to build — it is not a delete instruction'); +}); + +test('a real playlist still reclaims what it supersedes', async () => { + // The guard must not cost the feature it guards: a replace writes a NEW random filename, so the + // superseded copy lives at a different path and only the keep-set can find it. + const { content, post } = load(); + await seed(content, [A, B, OLD]); + post({ type: 'st-cache-playlist', urls: [A], prune: true }); + await settle(); + assert.deepEqual(keys(content), [A], 'everything the display no longer needs is dropped'); +}); + +test('an in-flight transfer\'s bookkeeping survives a prune of its own asset', async () => { + // The chunk keys are not in the keep-set (they carry __st_part), so deleting them on the URL test + // alone would restart that download on every 60s sweep — a resume that never completes. + const { content, post } = load(); + const chunk = A + '&__st_part=0'; + const meta = A + '&__st_part=meta'; + await seed(content, [B, chunk, meta]); + post({ type: 'st-cache-playlist', urls: [A], prune: true }); + await settle(); + assert.deepEqual(keys(content), [chunk, meta].sort(), 'progress on a wanted asset is kept; B is not wanted'); +}); + +test('prune:false never deletes, whatever the list says', async () => { + const { content, post } = load(); + await seed(content, [A, B]); + post({ type: 'st-cache-playlist', urls: [A], prune: false }); + await settle(); + assert.deepEqual(keys(content), [A, B].sort()); +}); + +test('a message that is not ours is ignored', async () => { + const { content, post } = load(); + await seed(content, [A, B]); + for (const bad of [null, {}, { type: 'other', urls: [], prune: true }, + { type: 'st-cache-playlist', prune: true }, { type: 'st-cache-playlist', urls: 'all', prune: true }]) { + post(bad); + } + await settle(); + assert.deepEqual(keys(content), [A, B].sort()); +}); From a1aeb324d7bbaeb4b210e3e6c8dc6c905cb97b23 Mon Sep 17 00:00:00 2001 From: ScreenTinker Date: Thu, 6 Aug 2026 16:26:43 -0500 Subject: [PATCH 2/2] Stop the worker claiming credit for offline widgets it never sees MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit sw.js said its cache-first widget branch "is what lets a widget keep rendering when the network is gone". It is not. The player mounts widgets in an iframe sandboxed to `allow-scripts` with no allow-same-origin, so the frame is an opaque-origin client, and a service worker does not control those — the navigation never reaches the handler. Measured rather than reasoned: a clock widget mounted five times over 25 seconds of real playback in Chrome while the shell cache held zero widget entries, and a plain fetch() of the identical URL from the controlled page was intercepted and stored on the first try. The branch works; the player's own widgets are simply not what reaches it. What actually holds widgets through an outage today is the HTTP cache plus the server's `max-age=31536000, immutable` on a rev-pinned render. That is sound in a desktop browser and is exactly the store this module's own header says is NOT persistent on BrightSign, which is why content caching had to exist at all. So the comment now records the limit and names the two ways out — route the render through a same-origin fetch and mount it as srcdoc, or grant allow-same-origin and hand widget scripts the player's origin, which is not a trade worth making for an offline nicety. The test pins the security property so nobody buys the cache with it, and pins the Cache-Control header, which is now known to be load-bearing on its own. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Uaeo9MvzKoyXuN6ZsbhtkL --- server/player/sw.js | 25 +++++++-- .../test/player-widget-frame-origin.test.js | 56 +++++++++++++++++++ 2 files changed, 76 insertions(+), 5 deletions(-) create mode 100644 server/test/player-widget-frame-origin.test.js diff --git a/server/player/sw.js b/server/player/sw.js index c369ae4..8e6b04f 100644 --- a/server/player/sw.js +++ b/server/player/sw.js @@ -54,11 +54,26 @@ self.addEventListener('fetch', (event) => { const url = new URL(event.request.url); // Widget renders pinned to a revision: cache-FIRST, because those exact bytes cannot change - // without the rev changing. This is what lets a widget keep rendering when the network is gone — - // previously the server sent no-store for every render, so widgets were the one thing the - // player's offline cache could never hold, and a display that lost its uplink lost them. - // ignoreSearch is deliberately NOT used here: the query string carries the rev, and ignoring it - // would match a different revision's entry, which is the staleness we are trying to remove. + // without the rev changing. ignoreSearch is deliberately NOT used here: the query string carries + // the rev, and ignoring it would match a different revision's entry, which is the staleness we + // are trying to remove. + // + // MEASURED LIMIT, do not read more into this branch than it delivers. The player mounts widgets in + // an iframe sandboxed to `allow-scripts` with NO allow-same-origin (index.html, + // renderWidgetBuffered), so that frame is an OPAQUE-origin client — and a service worker does not + // control opaque-origin clients. Its navigation request never reaches this handler. Driven in + // Chrome against a live server: a clock widget mounted five times over 25s and the shell cache + // held zero widget entries, while a plain fetch() of the identical URL from the controlled page + // was intercepted and stored. So this branch serves anything that reaches it — a same-origin + // fetch, a future non-sandboxed mount — and today the player's own widgets are not that. + // + // What actually keeps widgets rendering offline right now is the HTTP cache plus the server's + // `max-age=31536000, immutable` on a rev-pinned render (routes/widgets.js). That is sound in a + // desktop browser and is NOT a documented-persistent store on BrightSign, which guarantees + // survival across reboots for IndexedDB, localStorage and SQLite only — the same gap that made + // content caching necessary. Closing it properly means routing the render through a same-origin + // fetch and mounting it as srcdoc; granting the frame allow-same-origin instead would hand widget + // scripts the player's origin, which is not a trade worth making for an offline nicety. if (url.pathname.startsWith('/api/widgets/') && url.pathname.endsWith('/render') && url.searchParams.has('rev')) { event.respondWith( caches.match(event.request).then(cached => { diff --git a/server/test/player-widget-frame-origin.test.js b/server/test/player-widget-frame-origin.test.js new file mode 100644 index 0000000..fd71dac --- /dev/null +++ b/server/test/player-widget-frame-origin.test.js @@ -0,0 +1,56 @@ +'use strict'; + +// The widget iframe's origin, and what it costs. +// +// The player mounts a widget in an iframe sandboxed to `allow-scripts` with NO allow-same-origin, +// which gives it an OPAQUE origin: widget scripts cannot read the player's window, its localStorage, +// or its device token. That is deliberate and worth keeping — widget HTML is operator-authored and +//, for the webpage widget, third-party. +// +// It has a consequence that is easy to forget and was in fact forgotten: a service worker does not +// control opaque-origin clients, so the widget frame's navigation NEVER reaches sw.js. The +// cache-first widget branch there is real code that this player's own widgets do not use — measured +// in Chrome, a clock widget mounted five times over 25 seconds while the shell cache held zero +// widget entries, and a plain fetch() of the same URL from the controlled page was cached +// immediately. Widgets survive an outage today on the HTTP cache and the server's immutable +// Cache-Control, not on the worker. +// +// So this test pins the security property, and pins the fact that the offline story for widgets +// rests on the HTTP header. Someone who "fixes" offline widgets by adding allow-same-origin trades +// the isolation for a cache — the wrong direction, and the reason this is asserted rather than left +// as a comment. + +const { test } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); + +const HTML = fs.readFileSync(path.join(__dirname, '..', 'player', 'index.html'), 'utf8'); +const SW = fs.readFileSync(path.join(__dirname, '..', 'player', 'sw.js'), 'utf8'); +const WIDGETS = fs.readFileSync(path.join(__dirname, '..', 'routes', 'widgets.js'), 'utf8'); + +test('the widget iframe is sandboxed into an opaque origin', () => { + const sandboxes = [...HTML.matchAll(/setAttribute\('sandbox',\s*'([^']*)'\)/g)].map((m) => m[1]); + assert.ok(sandboxes.length > 0, 'the player must sandbox its widget frames'); + for (const s of sandboxes) { + assert.match(s, /allow-scripts/, 'a widget needs scripts to be a widget'); + assert.doesNotMatch(s, /allow-same-origin/, + 'allow-same-origin would give widget scripts the player origin — its storage, its device token'); + } +}); + +test('the offline guarantee for widgets is the HTTP header, and the server still sets it', () => { + // If this regresses to no-store, widgets stop surviving an outage everywhere — and the service + // worker will NOT quietly cover for it, because it never sees the request. + assert.match(WIDGETS, /max-age=31536000, immutable/, + 'a rev-pinned render must stay hard-cacheable: it is the only thing holding widgets offline'); + assert.match(WIDGETS, /no-store/, 'a render with no rev must stay uncacheable — nothing distinguishes one from the next'); +}); + +test('the worker does not claim to be what keeps widgets offline', () => { + // The comment above that branch used to say it was. A worker cannot control an opaque-origin + // client, so the claim was false in exactly the deployment it was written for. + const branch = SW.slice(0, SW.indexOf("url.pathname.startsWith('/api/widgets/')")); + assert.match(branch, /opaque-origin|opaque origin/i, + 'sw.js must record that the widget frame is opaque-origin and bypasses it'); +});