diff --git a/CHANGELOG.md b/CHANGELOG.md index 96034d7..c95df9a 100644 --- a/CHANGELOG.md +++ b/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/.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 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 5b59cc9..7a06cee 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/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 ce3d6ab..5ba0248 100644 --- a/server/server.js +++ b/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/.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/player-sw-scope.test.js b/server/test/player-sw-scope.test.js index 8e4e353..aaa7f77 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'); + // 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/); 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); +});