From 51b0b006b12c2c87b4152033135f2c29460916c3 Mon Sep 17 00:00:00 2001 From: screentinker Date: Mon, 13 Jul 2026 23:07:50 -0500 Subject: [PATCH] fix(pairing): reinstalled panel reclaims its device row instead of being blocked [Bold] (#180) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bold Media Group's fleet broke on the 1.9.3->1.9.6 upgrade. Their MDM does an uninstall/reinstall (app data wiped), so the player registers with { pairing_code, fingerprint } and NO device_id and shows a pairing code — but the dashboard reported "code does not exist". Deleting the device_fingerprints row fixed it, which pinpointed the fingerprint-reclaim guard in server/ws/deviceSocket.js. Root cause: the reclaim guard was `stillAlive = !!liveConn || secondsSince < reclaimSettleSeconds; if (stillAlive) reject`. On an in-place reinstall the old row heartbeat seconds ago, so `secondsSince < 300` is ALWAYS true -> it emitted device:auth-error and returned BEFORE the pairing_code INSERT, so the code the player displayed never existed server-side. The settle window's real purpose was to REMATCH an existing fingerprint back to its device row on reinstall — not to force a fresh re-pair. So the fix keys off claim status, not the timer (server-only; no APK change — reviewed and confirmed unnecessary): - Reject ONLY when the old row has a genuinely LIVE socket (liveConn) — the real anti- hijack boundary. Unchanged. - CLAIMED old row (user_id set) -> RECLAIM it regardless of the settle window: reuse the row, rotate the token, emit device:registered{online} + device:paired. The panel returns straight to paired (no operator re-pair, no orphaned duplicate row), preserving name / claim / playlist / content. device:paired drives the app off the pairing screen, so the fresh code it showed is irrelevant. - UNCLAIMED old row -> fall through to the pairing_code path and PROVISION FRESH with the shown code (reclaiming would leave a stale/null code -> "code does not exist"). #150 relinks the fingerprint to the new row. `reclaimSettleSeconds` is now vestigial for this path. Trade-off: a fingerprint-only reclaim of a CLAIMED-but-offline device is no longer delayed ~300s — not a new attack class (the old code already granted it once the window elapsed); liveConn remains the hard boundary. Truly closing that window without a re-pair needs client keystore attestation (a future APK). Also fixes a latent crash this newly exercises: middleware/subscription.js getUserPlan() dereferenced an undefined user in its else branch ("Cannot set properties of undefined (setting 'trial_active')") when the user/plan JOIN missed. Under the claimed-reclaim path that ran checkDeviceAccess->getUserPlan, the throw was swallowed by the reclaim try/catch and silently dropped the device to provision-fresh. Guard: `if (!user) return null`. Tests (server/test/fingerprint-reclaim.test.js): - NEW: a CLAIMED reinstall reclaims the SAME row, emits device:paired, creates no duplicate, keeps the fingerprint linked — regardless of the settle window (the Bold repro, fixed right). - NEW: recent heartbeat + no live socket, UNCLAIMED -> provisions fresh with the shown code. - NEW: a LIVE old socket still rejects and creates no new row (security preserved). - Updated the #143 gone-device test to expect provision-fresh for an unclaimed row, and the log-noise assertion to the "reclaim rejected" message. 465/465 server tests pass. Server-only: NOT deployed, no version bump, Android untouched. Co-authored-by: Claude Opus 4.8 --- server/middleware/subscription.js | 9 ++- server/test/fingerprint-reclaim.test.js | 82 +++++++++++++++++++++---- server/ws/deviceSocket.js | 37 +++++++++-- 3 files changed, 110 insertions(+), 18 deletions(-) diff --git a/server/middleware/subscription.js b/server/middleware/subscription.js index 2ccac89..56a7038 100644 --- a/server/middleware/subscription.js +++ b/server/middleware/subscription.js @@ -13,8 +13,15 @@ function getUserPlan(userId) { WHERE u.id = ? `).get(userId); + // No user row (or no joinable plan) — return null so callers treat it as unrestricted + // (checkDeviceAccess: `if (!plan) return { allowed: true }`). Previously the else branch + // below dereferenced an undefined `user` ("Cannot set properties of undefined"), which — once + // a claimed device's reclaim runs checkDeviceAccess — was swallowed by the caller's try/catch + // and silently dropped the device to the provision-fresh path instead of reclaiming it. + if (!user) return null; + // Check if trial has expired - if (user && user.trial_started) { + if (user.trial_started) { const trialEnd = user.trial_started + (TRIAL_DAYS * 86400); const now = Math.floor(Date.now() / 1000); user.trial_active = now < trialEnd; diff --git a/server/test/fingerprint-reclaim.test.js b/server/test/fingerprint-reclaim.test.js index 835ea32..7795fd7 100644 --- a/server/test/fingerprint-reclaim.test.js +++ b/server/test/fingerprint-reclaim.test.js @@ -41,9 +41,9 @@ before(async () => { after(() => { try { tdb && tdb.close(); } catch { /* */ } try { proc.kill('SIGKILL'); } catch { /* */ } }); // Seed a device + its fingerprint link directly (no socket -> no lingering liveConn). -function seedDevice(fp, { token, heartbeatAgo }) { +function seedDevice(fp, { token, heartbeatAgo, userId = null }) { const id = crypto.randomUUID(); - tdb.prepare("INSERT INTO devices (id, status, last_heartbeat, device_token) VALUES (?, 'offline', strftime('%s','now') - ?, ?)").run(id, heartbeatAgo, token); + tdb.prepare("INSERT INTO devices (id, status, last_heartbeat, device_token, user_id) VALUES (?, 'offline', strftime('%s','now') - ?, ?, ?)").run(id, heartbeatAgo, token, userId); tdb.prepare('INSERT INTO device_fingerprints (fingerprint, device_id) VALUES (?, ?)').run(fp, id); return { id, token }; } @@ -52,10 +52,12 @@ function staleHeartbeat(id, ago) { tdb.prepare("UPDATE devices SET last_heartbea function attempt(payload) { // one-shot register; resolves and closes return new Promise((resolve) => { const sock = ioClient(`${BASE}/device`, { transports: ['websocket'], reconnection: false, forceNew: true }); - const got = { registered: false, newId: null, authError: false, errorMsg: null }; + const got = { registered: false, newId: null, authError: false, errorMsg: null, paired: false, pairedId: null }; const finish = () => { try { sock.close(); } catch { /* */ } resolve(got); }; sock.on('connect', () => sock.emit('device:register', payload)); - sock.on('device:registered', (d) => { got.registered = true; got.newId = d.device_id; setTimeout(finish, 150); }); + // device:paired arrives right after device:registered on a claimed reclaim — wait 200ms to catch it. + sock.on('device:registered', (d) => { got.registered = true; got.newId = d.device_id; setTimeout(finish, 200); }); + sock.on('device:paired', (d) => { got.paired = true; got.pairedId = d && d.device_id; }); sock.on('device:auth-error', (e) => { got.authError = true; got.errorMsg = e && e.error; finish(); }); setTimeout(finish, 4000); }); @@ -71,13 +73,15 @@ function connectLive(payload) { // keeps the socket open (live connection); call } const rnd = () => String(crypto.randomInt(100000, 1000000)); -test('#143 repro: a gone device (no live conn + stale heartbeat) is reclaimable', async () => { +test('#143 no reclaim-loop: an UNCLAIMED gone device provisions fresh with the shown code', async () => { const fp = 'fp-gone-' + crypto.randomBytes(4).toString('hex'); - const dev = seedDevice(fp, { token: 'tok', heartbeatAgo: 99999 }); // ~27h stale, never connected - const r = await attempt({ pairing_code: rnd(), fingerprint: fp }); // no device_id -> reclaim path - assert.ok(r.registered, 'reclaim SUCCEEDS for a gone device'); - assert.equal(r.newId, dev.id, 'it reclaims the SAME device identity'); - assert.ok(!r.authError, 'no rejection'); + const dev = seedDevice(fp, { token: 'tok', heartbeatAgo: 99999 }); // unclaimed, ~27h stale, never connected + const code = rnd(); + const r = await attempt({ pairing_code: code, fingerprint: fp }); // no device_id + assert.ok(r.registered && !r.authError, 'registers cleanly — no stuck reclaim/retry loop (#143)'); + assert.notEqual(r.newId, dev.id, 'an UNCLAIMED old row is NOT reclaimed (its stale code would break pairing) — a fresh row is provisioned'); + const row = tdb.prepare('SELECT status FROM devices WHERE pairing_code = ?').get(code); + assert.ok(row && row.status === 'provisioning', 'the on-screen code is inserted as a fresh provisioning row'); }); test('no regression: a genuinely live device REJECTS a fingerprint reclaim', async () => { @@ -90,6 +94,60 @@ test('no regression: a genuinely live device REJECTS a fingerprint reclaim', asy try { live.sock.close(); } catch { /* */ } }); +// Bold 1.9.3->1.9.6 upgrade regression: a reinstall registers { pairing_code, fingerprint, +// no device_id } while the OLD device row heartbeat only seconds ago but has NO live socket +// (the app was uninstalled). The old guard treated the recent heartbeat as "still alive" and +// returned before the pairing_code INSERT, so the code the player displayed was never created +// and the dashboard said "code does not exist". It must now PROVISION FRESH with that code. +test('Bold upgrade: recent heartbeat + NO live socket provisions fresh with the shown code', async () => { + const fp = 'fp-upgrade-' + crypto.randomBytes(4).toString('hex'); + const dev = seedDevice(fp, { token: 'tokU', heartbeatAgo: 10 }); // heartbeat 10s ago, never socket-connected + const code = rnd(); + const r = await attempt({ pairing_code: code, fingerprint: fp }); // no device_id + assert.ok(r.registered, 'the reinstalled device provisions (device:registered), not blocked'); + assert.ok(!r.authError, 'not rejected by the reclaim-settle guard'); + const row = tdb.prepare('SELECT * FROM devices WHERE pairing_code = ?').get(code); + assert.ok(row, 'a devices row exists carrying the pairing_code the player is showing'); + assert.equal(row.status, 'provisioning', 'provisioned as a new, unclaimed device'); + assert.notEqual(row.id, dev.id, 'a fresh row — not a silent reclaim of the old identity'); + assert.equal(r.newId, row.id, 'the client is told its new device_id'); + const fpRow = tdb.prepare('SELECT device_id FROM device_fingerprints WHERE fingerprint = ?').get(fp); + assert.equal(fpRow.device_id, row.id, '#150: fingerprint relinked to the new device row'); +}); + +// The security boundary must survive the fix: if the OLD device still has a genuinely LIVE +// socket, a fingerprint clash must be rejected and NOT provision a new row. +test('security preserved: a LIVE old socket still rejects provisioning (no new row created)', async () => { + const fp = 'fp-live2-' + crypto.randomBytes(4).toString('hex'); + const dev = seedDevice(fp, { token: 'tokL', heartbeatAgo: 10 }); + const live = await connectLive({ device_id: dev.id, device_token: 'tokL', device_info: {} }); + assert.ok(live.registered, 'old device has a LIVE socket'); + const code = rnd(); + const r = await attempt({ pairing_code: code, fingerprint: fp }); + assert.ok(r.authError && !r.registered, 'rejected while the old device is genuinely live'); + const row = tdb.prepare('SELECT id FROM devices WHERE pairing_code = ?').get(code); + assert.ok(!row, 'no new device row is provisioned for a clash against a live device'); + try { live.sock.close(); } catch { /* */ } +}); + +// The claim-status fix: a CLAIMED panel that reinstalls (fingerprint only, no live socket) must +// REMATCH its existing row — preserve the claim/name/content, no operator re-pair, no orphaned +// duplicate — regardless of the settle window. This is what keeps a fleet upgrade seamless. +test('claimed reinstall RECLAIMS its row (no re-pair, no duplicate) regardless of the settle window', async () => { + const fp = 'fp-claimed-' + crypto.randomBytes(4).toString('hex'); + // CLAIMED (user_id set), heartbeat only 10s ago (well inside the old settle window), no live socket. + const dev = seedDevice(fp, { token: 'tokC', heartbeatAgo: 10, userId: 'user-' + crypto.randomBytes(3).toString('hex') }); + const before = tdb.prepare('SELECT COUNT(*) c FROM devices').get().c; + const code = rnd(); + const r = await attempt({ pairing_code: code, fingerprint: fp }); // reinstall: fingerprint only + assert.ok(r.registered && !r.authError, 'registers'); + assert.equal(r.newId, dev.id, 'rematches the SAME claimed device row — identity/claim preserved'); + assert.ok(r.paired, 'a claimed row emits device:paired, so the panel returns straight to paired (no code screen)'); + assert.equal(tdb.prepare('SELECT COUNT(*) c FROM devices').get().c, before, 'no new device row created (no fleet duplication)'); + assert.ok(!tdb.prepare('SELECT id FROM devices WHERE pairing_code = ?').get(code), 'the on-screen code is NOT provisioned as a separate row'); + assert.equal(tdb.prepare('SELECT device_id FROM device_fingerprints WHERE fingerprint = ?').get(fp).device_id, dev.id, 'the fingerprint stays linked to the reclaimed row'); +}); + test('clear-on-leave: after disconnect, liveConn is cleared so a (stale) device reclaims', async () => { const fp = 'fp-leave-' + crypto.randomBytes(4).toString('hex'); const dev = seedDevice(fp, { token: 'tok3', heartbeatAgo: 99999 }); @@ -113,6 +171,6 @@ test('log noise: a retried reclaim logs at most once per device per window', asy for (let i = 0; i < 4; i++) { const r = await attempt({ pairing_code: rnd(), fingerprint: fp }); assert.ok(r.authError, 'each retry is deferred'); } try { live.sock.close(); } catch { /* */ } await sleep(200); - const lines = fs.readFileSync(LOG, 'utf8').split('\n').filter(l => l.includes('reclaim deferred for ' + dev.id)).length; - assert.ok(lines <= 1, `at most one deferral log per window (got ${lines}); no double-log / per-2s flood`); + const lines = fs.readFileSync(LOG, 'utf8').split('\n').filter(l => l.includes('reclaim rejected for ' + dev.id)).length; + assert.ok(lines <= 1, `at most one rejection log per window (got ${lines}); no double-log / per-2s flood`); }); diff --git a/server/ws/deviceSocket.js b/server/ws/deviceSocket.js index 2fbba78..80e272d 100644 --- a/server/ws/deviceSocket.js +++ b/server/ws/deviceSocket.js @@ -470,20 +470,41 @@ module.exports = function setupDeviceSocket(io) { const liveConn = heartbeat.getConnection(existing.device_id); const lastBeat = oldDevice.last_heartbeat || 0; const secondsSince = Math.floor(Date.now() / 1000) - lastBeat; - const stillAlive = !!liveConn || secondsSince < config.reclaimSettleSeconds; - if (stillAlive) { + // Bold 1.9.3->1.9.6 upgrade fix: a reinstalled panel registers with + // { pairing_code, fingerprint } and NO device_id — it is asking to be + // PROVISIONED, not to reclaim. The ONLY grounds to reject is a genuinely + // LIVE socket on the old device row (stops a duplicated fingerprint from + // hijacking an actively-connected display). The previous guard also rejected + // on a merely-recent heartbeat (secondsSince < reclaimSettleSeconds), which + // on an in-place-reinstall upgrade is ALWAYS true — so it returned before the + // pairing_code INSERT and the code the player was displaying was never created, + // leaving the dashboard with "code does not exist" until device_fingerprints + // was cleared by hand. The real security boundary is the operator claiming the + // code in the dashboard; a cloned fingerprint that falls through only gets an + // unclaimed, tokenless, content-less row — harmless. + if (liveConn) { // Log at most once per device per window so a retrying/stuck device can't flood stdout. const nowMs = Date.now(); if (nowMs - (lastReclaimRejectLogAt.get(existing.device_id) || 0) >= config.reclaimRejectLogWindowMs) { lastReclaimRejectLogAt.set(existing.device_id, nowMs); - console.warn(`Fingerprint reclaim deferred for ${existing.device_id}: still settling (status=${oldDevice.status}, ${secondsSince}s since heartbeat, liveConn=${!!liveConn}); reclaimable after ${config.reclaimSettleSeconds}s offline`); + console.warn(`Fingerprint reclaim rejected for ${existing.device_id}: old device has a LIVE socket (status=${oldDevice.status}, ${secondsSince}s since heartbeat)`); } socket.emit('device:auth-error', { - error: `This display was recently active. If you reinstalled the app, retry after it has been offline for ${config.reclaimSettleSeconds} seconds.` + error: 'This display is currently active on another connection.' }); return; } - lastReclaimRejectLogAt.delete(existing.device_id); // reclaim proceeding — clear any deferral log state + // No live socket from here on — the old connection is gone. + lastReclaimRejectLogAt.delete(existing.device_id); + if (oldDevice.user_id) { + // The old row is CLAIMED. A reinstalled panel (MDM wiped its data, so it presents + // only its stable fingerprint — no device_id, no token) must REMATCH to this row — + // preserving the claim, name, playlist, and content — instead of provisioning a + // stranger the operator has to re-pair across the whole fleet. device:paired below + // drives the app off the pairing screen, so the fresh code it was displaying is + // irrelevant. We reclaim regardless of the settle window: liveConn (checked above) + // is the real anti-hijack boundary; the settle delay only postponed a + // fingerprint-only reclaim the server already grants once the window elapses. // Fingerprint matched — this is a reinstalled app reconnecting to its old device. // Issue a fresh token so the app can authenticate going forward. @@ -525,6 +546,12 @@ module.exports = function setupDeviceSocket(io) { socket.emit('device:playlist-update', buildPlaylistPayload(existing.device_id)); } return; + } + // The old row is UNCLAIMED (never paired). Reclaiming it would leave it carrying a + // stale/null pairing_code while the player shows a fresh one -> "code does not exist". + // Fall through to the pairing_code path below, which provisions a fresh row with the + // on-screen code and (#150) relinks the fingerprint to it. reclaimSettleSeconds no + // longer gates this path — a genuinely live socket (above) is the only rejection. } } } else if (device_id || pairing_code) {