diff --git a/server/lib/bounded-snapshot-store.js b/server/lib/bounded-snapshot-store.js new file mode 100644 index 0000000..48b1002 --- /dev/null +++ b/server/lib/bounded-snapshot-store.js @@ -0,0 +1,72 @@ +'use strict'; + +// A bounded "latest snapshot per key" store for diagnostic data that arrives from +// UNAUTHENTICATED callers. +// +// The widget-telemetry store was a plain Map keyed on a value the caller supplies, with no +// cap, no TTL and no eviction: an unauthenticated caller could add entries until the +// process ran out of memory, and on this product a dead server means every screen in the +// fleet reconnects at once. A bound is therefore a fleet-safety control, not tidiness. +// +// Deliberately NOT rate-limited per IP. This codebase already learned that lesson for the +// OTA download guard ("NEVER per-IP (SNAT)"): signage sites egress through one NAT +// address, so a per-IP cap punishes a whole venue for one noisy panel and does nothing +// against a distributed writer. The GLOBAL entry cap is the honest bound — it makes the +// worst case a fixed amount of memory regardless of who is writing or from where. +// +// Eviction is least-recently-WRITTEN. Every live reporter rewrites its own key on each +// report, so under a flood the only entries eligible for eviction are ones already older +// than any consumer would treat as live. + +function createStore({ max = 500, ttlMs = 60_000 } = {}) { + // Map preserves insertion order, and re-setting a key does NOT refresh that order, so + // delete-then-set is what makes the iteration order a true recency order. + const m = new Map(); + let sweepTimer = null; + + function set(key, value) { + if (m.has(key)) m.delete(key); + m.set(key, value); + // Evict the least-recently-written entries until we are back inside the cap. + while (m.size > max) { + const oldest = m.keys().next(); + if (oldest.done) break; + m.delete(oldest.value); + } + return value; + } + + // Returns null for a missing OR expired entry. Expiry is enforced on READ as well as by + // the sweep, so a stale value can never be served just because the sweep hasn't run. + function get(key, now = Date.now()) { + const v = m.get(key); + if (!v) return null; + const at = typeof v.receivedAt === 'number' ? v.receivedAt : 0; + if (now - at > ttlMs) { m.delete(key); return null; } + return v; + } + + function sweep(now = Date.now()) { + let dropped = 0; + for (const [k, v] of m) { + const at = typeof v.receivedAt === 'number' ? v.receivedAt : 0; + if (now - at > ttlMs) { m.delete(k); dropped++; } + } + return dropped; + } + + // unref() so the interval never holds the process open (same discipline as + // lib/log-coalescer.js and the other sweeps). + function startSweep(intervalMs = ttlMs) { + if (sweepTimer) return sweepTimer; + sweepTimer = setInterval(() => { try { sweep(); } catch (_) { /* never throw from a timer */ } }, intervalMs); + if (sweepTimer.unref) sweepTimer.unref(); + return sweepTimer; + } + + function stopSweep() { if (sweepTimer) { clearInterval(sweepTimer); sweepTimer = null; } } + + return { set, get, sweep, startSweep, stopSweep, size: () => m.size, max, ttlMs }; +} + +module.exports = { createStore }; diff --git a/server/routes/widgets.js b/server/routes/widgets.js index 0f80ad6..d3cb8a5 100644 --- a/server/routes/widgets.js +++ b/server/routes/widgets.js @@ -224,7 +224,19 @@ router.get('/:id/data.json', (req, res) => { // Latest frame-rate telemetry per widget, reported by the diag-smoothness widget running on a device. // In-memory (diagnostic, not persisted) — a device page reads the snapshot for the widget it plays. -const widgetTelemetry = new Map(); +// +// BOUNDED, because the writer is unauthenticated (the widget runs in a null-origin sandboxed +// iframe and cannot carry a session) and the key comes from the request body. An uncapped map +// keyed on caller-supplied values is a remote memory-exhaustion path, and on this product a dead +// server means the whole fleet reconnects at once. +// +// The cap is GLOBAL rather than per-IP on purpose: signage sites egress through one NAT address, +// so a per-IP limit would punish an entire venue for one noisy panel while doing nothing about a +// distributed writer. Same reasoning as lib/ota-download-guard ("NEVER per-IP (SNAT)"). Eviction +// is least-recently-written, and a live panel rewrites its key every 2.5s, so only entries the +// dashboard would already call stale (>15s) are ever eligible. +const widgetTelemetry = require('../lib/bounded-snapshot-store').createStore({ max: 500, ttlMs: 60_000 }); +widgetTelemetry.startSweep(); // Public POST from the widget: it runs in a null-origin sandboxed iframe, so this must be no-auth + // CORS-open. The widget sends text/plain (a "simple" request → no CORS preflight); we JSON.parse it. router.post('/:id/telemetry', express.text({ type: '*/*', limit: '16kb' }), (req, res) => { @@ -236,7 +248,11 @@ router.post('/:id/telemetry', express.text({ type: '*/*', limit: '16kb' }), (req // fall back to a widget-scoped key for players that don't pass a device id yet. const key = (t.device && String(t.device).slice(0, 64)) || ('w:' + req.params.id); widgetTelemetry.set(key, t); - res.json({ ok: true }); + // 204, not res.json(): this is fire-and-forget diagnostic data and the reporting widget ignores + // the response entirely (routes/widgets.js renderDiagSmoothness -> fetch(...).catch()). It also + // keeps services/activity.js activityLogger — which wraps res.json — from writing an activity_log + // row per unauthenticated report, i.e. from letting an anonymous caller grow a DB table. + res.status(204).end(); }); // Public GET so the dashboard device page can display the snapshot. ?device= reads that panel's // report; without it (or if that panel hasn't reported) falls back to the widget-scoped snapshot. @@ -247,8 +263,10 @@ router.get('/:id/telemetry', (req, res) => { // Device-scoped request returns ONLY that device's report — NO widget-wide fallback, or one // reporting panel's data would show on every other device's page (incl. offline ones). A request // with no device id gets the widget-scoped snapshot (raw/debug view only). - const rec = dev ? (widgetTelemetry.get(dev) || null) : (widgetTelemetry.get('w:' + req.params.id) || null); - res.json(rec); + // get() returns null for a missing OR expired entry, so a stale snapshot is never served + // as live even between sweeps. + const rec = dev ? widgetTelemetry.get(dev) : widgetTelemetry.get('w:' + req.params.id); + res.json(rec || null); }); // Preview unsaved widget from config (used by editor Preview button) diff --git a/server/test/widget-telemetry-bounded.test.js b/server/test/widget-telemetry-bounded.test.js new file mode 100644 index 0000000..fe63b34 --- /dev/null +++ b/server/test/widget-telemetry-bounded.test.js @@ -0,0 +1,144 @@ +'use strict'; + +// The widget telemetry endpoint accepts writes from UNAUTHENTICATED callers (the diag +// widget runs in a null-origin sandboxed iframe, so it cannot carry a session). Two +// invariants keep that from being a resource-exhaustion path — on this product a dead +// server is a fleet-wide reconnect, so these are fleet-safety properties: +// +// 1. The in-memory store is BOUNDED — a fixed entry cap and a TTL, so an unauthenticated +// writer cannot grow it without limit no matter how many distinct keys it invents. +// 2. An unauthenticated report writes NO durable row — it must not be able to grow a +// database table either. +// +// And the consumer contract is unchanged: a live key returns its object, an unknown or +// expired key returns null, which frontend/js/views/device-detail.js already handles +// (it renders "no report yet" and treats anything older than 15s as stale anyway). + +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 { createStore } = require('../lib/bounded-snapshot-store'); + +// --------------------------------------------------------------------------- +// 1. The bound itself (unit) +// --------------------------------------------------------------------------- +test('the store never exceeds its cap, however many distinct keys arrive', () => { + const s = createStore({ max: 50, ttlMs: 60_000 }); + for (let i = 0; i < 5000; i++) s.set('attacker-key-' + i, { receivedAt: Date.now() }); + assert.equal(s.size(), 50, '5000 distinct keys must not produce 5000 entries'); +}); + +test('eviction is least-recently-written, so a live reporter is never dropped', () => { + const s = createStore({ max: 10, ttlMs: 60_000 }); + s.set('live-panel', { receivedAt: Date.now() }); + for (let i = 0; i < 100; i++) { + s.set('noise-' + i, { receivedAt: Date.now() }); + s.set('live-panel', { receivedAt: Date.now() }); // the panel keeps reporting + } + assert.ok(s.get('live-panel'), 'a key that keeps being written survives a flood'); + assert.equal(s.size(), 10); +}); + +test('entries expire, on read as well as by sweep', () => { + const s = createStore({ max: 100, ttlMs: 1000 }); + const t0 = 1_000_000; + s.set('k', { receivedAt: t0 }); + assert.ok(s.get('k', t0 + 500), 'fresh entry is returned'); + assert.equal(s.get('k', t0 + 5000), null, 'expired entry reads as null, not stale data'); + + s.set('a', { receivedAt: t0 }); + s.set('b', { receivedAt: t0 }); + assert.equal(s.sweep(t0 + 5000), 2, 'sweep drops expired entries'); + assert.equal(s.size(), 0); +}); + +test('a missing key reads as null — the shape the dashboard already handles', () => { + const s = createStore(); + assert.equal(s.get('never-seen'), null); +}); + +test('the sweep timer does not hold the process open', () => { + const s = createStore({ ttlMs: 50 }); + const t = s.startSweep(10); + assert.equal(typeof t.unref, 'function'); + s.stopSweep(); +}); + +// --------------------------------------------------------------------------- +// 2. End to end: no durable row, and the HTTP contract is unchanged +// --------------------------------------------------------------------------- +const { freePort } = require('./helpers/free-port'); +let PORT, BASE, proc; +const DATA_DIR = path.join(os.tmpdir(), 'st-telemetry-test-' + crypto.randomBytes(4).toString('hex')); +const LOG = path.join(os.tmpdir(), 'st-telemetry-' + crypto.randomBytes(4).toString('hex') + '.log'); + +before(async () => { + PORT = await freePort(); + BASE = `http://127.0.0.1:${PORT}`; + const logFd = fs.openSync(LOG, 'w'); + 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', logFd, logFd], + }); + for (let i = 0; i < 80; i++) { + try { const r = await fetch(BASE + '/api/status'); if (r.ok) return; } catch { /* not yet */ } + await new Promise(r => setTimeout(r, 250)); + } + throw new Error('server did not boot:\n' + fs.readFileSync(LOG, 'utf8').slice(-2000)); +}); +after(() => { try { proc.kill('SIGKILL'); } catch { /* ignore */ } }); + +const postTelemetry = (widgetId, body) => fetch(`${BASE}/api/widgets/${widgetId}/telemetry`, { + method: 'POST', headers: { 'Content-Type': 'text/plain' }, body: JSON.stringify(body), +}); + +test('an unauthenticated report writes no durable row', async () => { + const wid = 'w-' + crypto.randomBytes(4).toString('hex'); + for (let i = 0; i < 25; i++) { + const res = await postTelemetry(wid, { device: 'dev-' + i, fps: 60 }); + assert.ok(res.status < 400, 'the widget must keep being able to report'); + } + const Database = require('better-sqlite3'); + const db = new Database(path.join(DATA_DIR, 'db', 'remote_display.db'), { readonly: true }); + const n = db.prepare("SELECT COUNT(*) n FROM activity_log WHERE action LIKE '%telemetry%'").get().n; + db.close(); + assert.equal(n, 0, 'an unauthenticated caller must not be able to grow activity_log'); +}); + +test('the SERVER store is bounded — a flood of distinct keys evicts older ones', async () => { + // The unit tests above specify the store in isolation; this one proves it is actually + // WIRED IN, by observing eviction through the HTTP surface. On an unbounded store the + // first key survives forever and this fails. + const wid = 'w-' + crypto.randomBytes(4).toString('hex'); + const first = 'dev-first-' + crypto.randomBytes(4).toString('hex'); + await postTelemetry(wid, { device: first, fps: 1 }); + assert.ok((await (await fetch(`${BASE}/api/widgets/${wid}/telemetry?device=${first}`)).json()), + 'the first report is readable before the flood'); + + for (let i = 0; i < 700; i++) await postTelemetry(wid, { device: `flood-${i}`, fps: 60 }); + + const after = await (await fetch(`${BASE}/api/widgets/${wid}/telemetry?device=${first}`)).json(); + assert.equal(after, null, 'an unbounded store would still be holding the first key'); +}); + +test('the read contract is unchanged: live key -> object, unknown key -> null', async () => { + const wid = 'w-' + crypto.randomBytes(4).toString('hex'); + const dev = 'dev-' + crypto.randomBytes(4).toString('hex'); + await postTelemetry(wid, { device: dev, fps: 59, verdict: 'SMOOTH' }); + + const live = await fetch(`${BASE}/api/widgets/${wid}/telemetry?device=${dev}`); + assert.equal(live.status, 200); + const body = await live.json(); + assert.equal(body.fps, 59, 'the reporting panel\'s snapshot comes back'); + assert.equal(body.verdict, 'SMOOTH'); + assert.equal(typeof body.receivedAt, 'number', 'receivedAt drives the dashboard staleness check'); + + const unknown = await fetch(`${BASE}/api/widgets/${wid}/telemetry?device=nobody`); + assert.equal(unknown.status, 200); + assert.equal(await unknown.json(), null, 'unknown device reads as null, the shape the UI handles'); +});