From e812f35b6bd069e26db3fc9643adb404d43d2826 Mon Sep 17 00:00:00 2001 From: ScreenTinker Date: Thu, 6 Aug 2026 16:31:53 -0500 Subject: [PATCH 1/2] Fail loudly on a missing asset, and stop an empty playlist wiping the cache MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two faults that are live on 1.9.29, both silent, both ending in a dark screen. A missing upload answered 200 OK with Content-Type: text/html and 15KB of the dashboard, under the immutable/30-day header the mount sets before it knows whether the file exists. Every downloader here treats 200 as success, so a panel stores the page AS the video and caches it for a month; Android validates the byte count, not the type, so a correctly-sized page passes integrity and is promoted as a valid asset. Reachable exactly when it hurts — a replace writes a new random filename and unlinks the old one. Now a 404, with the cache header removed. And the service worker treated an empty playlist as "keep nothing". But `assignments: []` is what the server sends for a device between playlists, for a playlist never published, and from the catch when a snapshot fails to parse — so a message that means nothing of the sort deleted every byte of media the panel held. Only survivable while the uplink is up, i.e. exactly when the cache is worthless. Both regression tests drive the whole server or the real worker, because both bugs live in the relationship between two pieces that are individually correct: the order of two mounts, and the difference between "needs nothing" and "did not arrive". Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Uaeo9MvzKoyXuN6ZsbhtkL --- CHANGELOG.md | 31 +++++ server/player/sw.js | 17 ++- server/server.js | 23 +++- server/test/sw-prune-guard.test.js | 123 +++++++++++++++++++ server/test/uploads-missing-file-404.test.js | 81 ++++++++++++ 5 files changed, 272 insertions(+), 3 deletions(-) create mode 100644 server/test/sw-prune-guard.test.js create mode 100644 server/test/uploads-missing-file-404.test.js diff --git a/CHANGELOG.md b/CHANGELOG.md index 6db2594..0dd4d01 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,36 @@ # Changelog +## 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/.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 The release candidates 1.9.29-rc1 through rc5 are folded in here; the entries below record what diff --git a/server/player/sw.js b/server/player/sw.js index 806f8a2..c83c303 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,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; diff --git a/server/server.js b/server/server.js index b3bccae..294b8a2 100644 --- a/server/server.js +++ b/server/server.js @@ -966,7 +966,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/.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 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()); +}); diff --git a/server/test/uploads-missing-file-404.test.js b/server/test/uploads-missing-file-404.test.js new file mode 100644 index 0000000..2694bdc --- /dev/null +++ b/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/.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); +}); From e313826d85eef70e8918a405c99d154d4a0a589b Mon Sep 17 00:00:00 2001 From: ScreenTinker Date: Thu, 6 Aug 2026 16:32:23 -0500 Subject: [PATCH 2/2] chore(release): v1.9.30 --- VERSION | 2 +- docs/openapi.yaml | 2 +- server/package-lock.json | 4 ++-- server/package.json | 2 +- server/test/player-sw-scope.test.js | 7 ++++++- 5 files changed, 11 insertions(+), 6 deletions(-) diff --git a/VERSION b/VERSION index 5620bb8..d682215 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -1.9.29 +1.9.30 diff --git a/docs/openapi.yaml b/docs/openapi.yaml index 097da3e..dcc68ca 100644 --- a/docs/openapi.yaml +++ b/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. diff --git a/server/package-lock.json b/server/package-lock.json index 2c871d1..a6ffbd7 100644 --- a/server/package-lock.json +++ b/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", diff --git a/server/package.json b/server/package.json index 821d554..9abc9db 100644 --- a/server/package.json +++ b/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": { diff --git a/server/test/player-sw-scope.test.js b/server/test/player-sw-scope.test.js index b4c7c46..f0af2d8 100644 --- a/server/test/player-sw-scope.test.js +++ b/server/test/player-sw-scope.test.js @@ -66,7 +66,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/);