Merge 1.9.30: fail loudly on a missing asset; stop an empty playlist wiping the cache

# Conflicts:
#	CHANGELOG.md

CHANGELOG.md

@@ -27,6 +27,36 @@ wall was invisible from the dashboard, and inspecting a single screen meant pull
 wall (re-syncing the live wall) and putting it back. The wall screen now lists its panels with live
 online state and a link straight to each device's page, and the wall card on the dashboard shows a
 per-member status chip. A screenshot can be requested per panel without disturbing playback.
+## 1.9.30
+
+A patch off 1.9.29 carrying two fixes for faults that are live and silent. Both were found by a QA
+pass driving real browsers rather than by reading code, and both fail in the direction that leaves a
+screen dark with nothing in any log.
+
+### Fixed — a missing media file answered 200 with the dashboard, cached for a month
+`express.static` calls `next()` on a miss and the only thing downstream was the SPA catch-all, so
+`GET /uploads/content/<gone>.mp4` returned **200 OK, `Content-Type: text/html`**, 15KB of
+`index.html`, under the `public, max-age=2592000, immutable` header the mount had already set on the
+way in.
+
+For a player that is the worst possible answer. Every downloader in this product treats 200 as
+success, so a panel stores the HTML page **as the video**, caches it for a month, and renders a black
+frame. Android's cache validates the byte COUNT against `Content-Length`, not the content type, so a
+correctly-sized page passes the integrity check and is promoted as a valid asset.
+
+It is reachable exactly when it hurts: a content replace writes a new randomly-named file and unlinks
+the old one, so any snapshot still pointing at the old name asks for a file that is gone. A miss now
+terminates in a 404 with no cache header — `immutable` is a promise about a file that exists.
+
+### Fixed — an empty playlist wiped a display's entire offline library
+The player asks the service worker to hold its current media and to drop anything else. An empty list
+was honoured as "drop everything" — and `assignments: []` is what the server sends for a device
+between playlists, for a playlist never published, and from inside the `catch` when a stored snapshot
+fails to parse. 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 does not
+matter. A cache kept too long costs disk the quota reclaims anyway; one dropped at the wrong moment
+is a dark screen with no way back. An empty list is no longer a prune instruction.
 
 ## 1.9.29
 

VERSION

@@ -1 +1 @@
-1.9.29
+1.9.30

docs/openapi.yaml

@@ -1,7 +1,7 @@
 openapi: 3.1.0
 info:
   title: ScreenTinker Public API
-  version: 1.9.29
+  version: 1.9.30
   description: |
     Public, token-scoped REST API for ScreenTinker digital signage.
 

server/package-lock.json

@@ -1,12 +1,12 @@
 {
   "name": "screentinker",
-  "version": "1.9.29",
+  "version": "1.9.30",
   "lockfileVersion": 3,
   "requires": true,
   "packages": {
     "": {
       "name": "screentinker",
-      "version": "1.9.29",
+      "version": "1.9.30",
       "dependencies": {
         "@azure/msal-node": "^5.2.1",
         "archiver": "^7.0.1",

server/package.json

@@ -1,6 +1,6 @@
 {
   "name": "screentinker",
-  "version": "1.9.29",
+  "version": "1.9.30",
   "description": "ScreenTinker - Digital Signage Management Server",
   "main": "server.js",
   "scripts": {

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,17 @@ 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 is not rare: buildPlaylistPayload() yields `assignments: []` for a device between
+  // playlists, for a playlist never published, AND inside the catch when a published_snapshot fails
+  // to JSON.parse. Honouring it deleted every byte of media the panel held — 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 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;

server/server.js

@@ -996,7 +996,28 @@ app.use('/uploads/content', (req, res, next) => {
     // re-assert the override here for anything not inline-safe.
     hardenUploadResponse(res, filePath);
   },
-}));
+}), (req, res) => {
+  /*
+   * A miss ENDS here. express.static calls next() when the file isn't there, and the only thing
+   * left downstream is the SPA catch-all — so GET /uploads/content/<gone>.mp4 answered 200
+   * text/html with 15KB of dashboard, under the `immutable, max-age=30d` header this middleware
+   * had already set on the way in.
+   *
+   * That is the worst possible answer for a player. Every downloader treats 200 as success, so the
+   * panel stores the HTML page AS the video, caches it for a month, and plays a black frame with
+   * nothing in any log to say why. Android's cache validates the BYTE COUNT, not the type, so a
+   * correctly-sized page passes the integrity check and is promoted as a valid asset.
+   *
+   * It is reachable the moment an asset is replaced (a replace writes a new random filename and
+   * unlinks the old one) or a file goes missing from the volume — exactly when a screen most needs
+   * to fail loudly.
+   *
+   * The Cache-Control goes too: 'immutable' is a promise about a file that exists.
+   */
+  res.removeHeader('Cache-Control');
+  res.removeHeader('Content-Disposition');
+  res.type('application/json').status(404).json({ error: 'Not found' });
+});
 
 // Media proxy for remote (URL-referenced) playlist items — public by construction (players are
 // unauthenticated browsers). Takes an itemId, never a caller URL: it fetches the item's stored

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');
+  // The guard is `data.prune && data.urls.length > 0`, not a bare `data.prune`: an EMPTY list is
+  // never a prune instruction. `assignments: []` is what the server sends for a device between
+  // playlists and from the catch when a snapshot fails to parse, and honouring it as "keep nothing"
+  // wiped a panel's whole offline library (fixed in 1.9.30).
+  assert.match(sw, /if \(data\.prune && data\.urls\.length > 0\)/,
+    'the prune must require a NON-EMPTY declared set');
 
   const html = fs.readFileSync(path.join(__dirname, '..', 'player', 'index.html'), 'utf8');
   assert.match(html, /prune:\s*true/);

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());
+});

server/test/uploads-missing-file-404.test.js

@@ -0,0 +1,81 @@
+'use strict';
+
+/*
+ * A missing upload must 404, not hand the player the dashboard.
+ *
+ * express.static calls next() on a miss, and the only thing downstream of /uploads/content was the
+ * SPA catch-all. So GET /uploads/content/<gone>.mp4 answered 200 OK, Content-Type: text/html, with
+ * 15KB of index.html — under the `public, max-age=2592000, immutable` header the mount had already
+ * set on the way in, before it knew whether the file existed.
+ *
+ * For a player that is the worst possible answer. Every downloader treats 200 as success, so the
+ * panel stores the HTML page AS the video, caches it for a month, and renders a black frame with
+ * nothing in any log to explain it. And it is reachable exactly when it hurts: a content REPLACE
+ * writes a new randomly-named file and unlinks the old one, so every snapshot still pointing at the
+ * old name asks for a file that is gone.
+ *
+ * Whole-server test on purpose — the bug was the ORDER of two mounts, which no unit test can see.
+ */
+
+const { test, before, after } = require('node:test');
+const assert = require('node:assert/strict');
+const { spawn } = require('node:child_process');
+const path = require('node:path');
+const os = require('node:os');
+const fs = require('node:fs');
+const crypto = require('node:crypto');
+const { freePort } = require('./helpers/free-port');
+
+const DATA_DIR = path.join(os.tmpdir(), 'st-uploads404-' + crypto.randomBytes(4).toString('hex'));
+const sleep = (ms) => new Promise((r) => setTimeout(r, ms));
+let proc, BASE;
+
+before(async () => {
+  const PORT = await freePort();
+  BASE = `http://127.0.0.1:${PORT}`;
+  proc = spawn('node', ['server.js'], {
+    cwd: path.join(__dirname, '..'),
+    env: { ...process.env, DATA_DIR, SELF_HOSTED: 'true', PORT: String(PORT), NODE_ENV: 'test' },
+    stdio: 'ignore',
+  });
+  for (let i = 0; i < 100; i++) {
+    try { const r = await fetch(BASE + '/api/status'); if (r.ok) break; } catch { /* booting */ }
+    await sleep(150);
+  }
+});
+
+after(async () => { if (proc) proc.kill('SIGKILL'); await sleep(150); });
+
+test('a missing media file is a 404, not 200 with an HTML page', async () => {
+  const res = await fetch(BASE + '/uploads/content/00000000-0000-0000-0000-000000000000.mp4');
+  assert.equal(res.status, 404);
+  const ct = res.headers.get('content-type') || '';
+  assert.ok(!ct.includes('text/html'), `a player must never be handed HTML as a video (got ${ct})`);
+});
+
+test('and it is not cached for a month — immutable is a promise about a file that exists', async () => {
+  const res = await fetch(BASE + '/uploads/content/also-not-here.png');
+  assert.equal(res.status, 404);
+  const cc = res.headers.get('cache-control') || '';
+  assert.ok(!cc.includes('immutable'), `a 404 must not be cached as the asset (got "${cc}")`);
+});
+
+test('a file that IS there still serves, with its own type and the long cache', async () => {
+  // The guard must terminate ONLY the miss. A 404 on a present file would black out every screen.
+  const contentDir = path.join(DATA_DIR, 'uploads', 'content');
+  fs.mkdirSync(contentDir, { recursive: true });
+  const name = 'present-' + crypto.randomBytes(4).toString('hex') + '.png';
+  const png = Buffer.from('89504e470d0a1a0a0000000d49484452', 'hex');
+  fs.writeFileSync(path.join(contentDir, name), png);
+
+  const res = await fetch(`${BASE}/uploads/content/${name}`);
+  assert.equal(res.status, 200);
+  assert.equal(res.headers.get('content-type'), 'image/png');
+  assert.ok((res.headers.get('cache-control') || '').includes('immutable'), 'real assets keep the 30-day cache');
+  assert.equal(Buffer.from(await res.arrayBuffer()).length, png.length);
+});
+
+test('path traversal out of the content dir is still not reachable', async () => {
+  const res = await fetch(BASE + '/uploads/content/..%2f..%2f..%2fetc%2fpasswd');
+  assert.notEqual(res.status, 200);
+});