From a3b668d32f2e24cd8a75834b49b02e2bc27cb2ff Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 21:09:11 -0500 Subject: [PATCH] Treat an empty device_info as "nothing new", not as "forget what you know" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every web and BrightSign player nulled seventeen of its own device columns every five minutes. The browser player's refresh-register sends `device_info: {}` on a 300-second timer — it has nothing new to report, it just wants a fresh playlist. But `{}` is truthy, and applyDeviceInfo is a blind full-row overwrite with no per-field presence check, so it bound undefined for every column. better-sqlite3 stores undefined as NULL rather than throwing, so the write succeeded and the row was quietly emptied: android_version, app_version, screen_width/height, render_*, ota_status and attempts, tier, the four capability flags and the four volume/brightness columns. Android never hit it, because it always sends the full object. So this degraded exactly the client family that cannot be inspected any other way — a browser player has no adb, and the dashboard row is all there is. Fleet view, resolution diagnostics and any version-based logic read blank for them, which also makes evaluating a browser-based platform look worse than it is. The surrounding code already anticipates the refresh shape: recordReconnect and persistIdentity are both gated behind `if (!isPlaylistRefresh)`. This call was the one that was not. 5 tests, including one pinning the driver behaviour the bug depended on — undefined binds as NULL rather than throwing, which is why this was a silent five-minutely wipe instead of a loud error. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Uaeo9MvzKoyXuN6ZsbhtkL --- server/test/device-info-empty-refresh.test.js | 82 +++++++++++++++++++ server/ws/deviceSocket.js | 13 ++- 2 files changed, 94 insertions(+), 1 deletion(-) create mode 100644 server/test/device-info-empty-refresh.test.js diff --git a/server/test/device-info-empty-refresh.test.js b/server/test/device-info-empty-refresh.test.js new file mode 100644 index 0000000..357047e --- /dev/null +++ b/server/test/device-info-empty-refresh.test.js @@ -0,0 +1,82 @@ +'use strict'; + +// Every web and BrightSign player nulled seventeen of its own device columns every five minutes. +// +// The browser player's refresh-register sends `device_info: {}` on a 300-second timer — it has +// nothing new to report, it just wants a fresh playlist. But `{}` is truthy, and applyDeviceInfo is +// a blind full-row overwrite with no per-field presence check, so it bound `undefined` for every +// column. better-sqlite3 stores undefined as NULL rather than throwing, so the write succeeded and +// the row was quietly emptied: version, resolution, render size, OTA state, tier, the capability +// flags and the volume/brightness columns. +// +// Android was unaffected because it always sends the full object — so this only ever degraded the +// client family that has no other way to be inspected. Fleet view, resolution diagnostics and any +// version-based logic read blank for them. +// +// The surrounding code already anticipates this shape: recordReconnect and persistIdentity are +// gated behind `if (!isPlaylistRefresh)`. This one call was not. +// +// The invariant: an empty device_info means "nothing new", never "forget what you know". + +const { test } = require('node:test'); +const assert = require('node:assert/strict'); +const path = require('path'); +const fs = require('fs'); +const os = require('os'); + +const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'st-devinfo-')); +process.env.DATA_DIR = tmp; + +const { db } = require('../db/database'); + +const WS = 'ws-di', O = 'o-di', U = 'u-di', DEV = 'dev-di'; +db.prepare("INSERT OR IGNORE INTO users (id,email,password_hash) VALUES (?,?, 'x')").run(U, 'di@t.local'); +db.prepare('INSERT OR IGNORE INTO organizations (id,name,owner_user_id) VALUES (?,?,?)').run(O, 'Org', U); +db.prepare('INSERT OR IGNORE INTO workspaces (id,organization_id,name) VALUES (?,?,?)').run(WS, O, 'WS'); +db.prepare(`INSERT OR IGNORE INTO devices (id,name,workspace_id,android_version,app_version,screen_width,screen_height,created_at,updated_at) + VALUES (?, 'Web Screen', ?, 'Web/Chrome', '1.1.0-web', 1920, 1080, strftime('%s','now'), strftime('%s','now'))`).run(DEV, WS); + +const row = () => db.prepare('SELECT android_version, app_version, screen_width, screen_height FROM devices WHERE id = ?').get(DEV); + +// The guard as the socket handler applies it. +const shouldApply = (deviceInfo) => !!(deviceInfo && Object.keys(deviceInfo).length > 0); + +test('THE BUG: an empty device_info must not be treated as new information', () => { + // `{}` is truthy — that is the whole trap. + assert.equal(!!{}, true, 'this is why the old `if (device_info)` let it through'); + assert.equal(shouldApply({}), false, 'but it carries nothing, so nothing should be written'); +}); + +test('undefined really does become NULL rather than throwing, so the write did succeed', () => { + // Pinning the driver behaviour the bug depended on: had it thrown, this would have been loud + // instead of a silent five-minutely wipe. + const before = row(); + assert.equal(before.app_version, '1.1.0-web'); + db.prepare('UPDATE devices SET app_version = ? WHERE id = ?').run(undefined, DEV); + assert.equal(row().app_version, null, 'silently nulled — no error, no warning'); + db.prepare('UPDATE devices SET app_version = ? WHERE id = ?').run('1.1.0-web', DEV); +}); + +test('a refresh-register leaves what we already know intact', () => { + const before = row(); + if (shouldApply({})) throw new Error('guard failed'); // the handler would skip the write + const after = row(); + assert.deepEqual(after, before, 'version and resolution must survive a refresh beat'); +}); + +test('a real device_info is still applied', () => { + const info = { app_version: '1.9.28', screen_width: 3840 }; + assert.equal(shouldApply(info), true); + db.prepare('UPDATE devices SET app_version = ?, screen_width = ? WHERE id = ?') + .run(info.app_version, info.screen_width, DEV); + const after = row(); + assert.equal(after.app_version, '1.9.28'); + assert.equal(after.screen_width, 3840); +}); + +test('a missing device_info is skipped too', () => { + assert.equal(shouldApply(undefined), false); + assert.equal(shouldApply(null), false); +}); + +test.after(() => { try { fs.rmSync(tmp, { recursive: true, force: true }); } catch (_) {} }); diff --git a/server/ws/deviceSocket.js b/server/ws/deviceSocket.js index e82a10c..16776d5 100644 --- a/server/ws/deviceSocket.js +++ b/server/ws/deviceSocket.js @@ -782,7 +782,18 @@ module.exports = function setupDeviceSocket(io) { // null-token device" path is removed — that was the re-provisioning vector. const tokenToSend = device.device_token; - if (device_info) applyDeviceInfo(device_id, device_info); + // An EMPTY device_info means "I have nothing new to tell you", not "wipe what you know". + // The web/BrightSign player's refresh-register sends `device_info: {}` on a 300s timer, + // and `{}` is truthy — so every five minutes applyDeviceInfo, which is a blind full-row + // overwrite with no per-field presence check, bound `undefined` for all 17 columns. + // better-sqlite3 stores those as NULL rather than throwing, so the write succeeded: + // android_version, app_version, screen_width/height, render_*, ota_*, tier, the four + // capability flags and the four volume/brightness columns were all nulled. Fleet view, + // resolution diagnostics and version-based logic read blank for exactly the client family + // that cannot be inspected any other way. The code around this already anticipates the + // shape — recordReconnect/persistIdentity are gated behind `if (!isPlaylistRefresh)` — + // this call was the one that was not. + if (device_info && Object.keys(device_info).length > 0) applyDeviceInfo(device_id, device_info); heartbeat.registerConnection(device_id, socket.id); // #134: a same-socket re-register is a playlist REFRESH (~45-60s), NOT a reconnect and NOT